From: Amir Goldstein <amir73il@gmail.com>
To: Hugo Valtier <hugo@valtier.fr>
Cc: mfasheh@suse.de, viro@zeniv.linux.org.uk,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
Christian Brauner <brauner@kernel.org>
Subject: Re: bug in may_dedupe_file allows to deduplicate files we aren't allowed to write to
Date: Sat, 4 May 2024 12:43:44 +0300 [thread overview]
Message-ID: <CAOQ4uxjh5iQ0_knRebNRS271vR2-2f_9bNZyBG5vUy3rw6xh-g@mail.gmail.com> (raw)
In-Reply-To: <CAF+WW=oKQak6ktiOH75pHSDe7YEkYD-1ditgcsWB=z+aRKJogQ@mail.gmail.com>
On Sat, May 4, 2024 at 7:49 AM Hugo Valtier <hugo@valtier.fr> wrote:
>
> For context I am making a file based deduplication tool.
>
> I found that in this commit
> 5de4480ae7f8 ("vfs: allow dedupe of user owned read-only files")
> it states:
> > - the process could get write access
>
> However the behavior added in allow_file_dedupe now may_dedupe_file is opposite:
> > + if (!inode_permission(file_inode(file), MAY_WRITE))
> > + return true
>
> I've tested that I can create an other readonly file as root and have
> my unprivileged user deduplicate it however if I then make the file
> other writeable I cannot anymore*.
> It doesn't make sense to me why giving write permissions on a file
> should remove the permission to deduplicate*.
True. Here is the discussion about adding "could have been opened w"
to allow dedupe:
https://lore.kernel.org/linux-fsdevel/20180517230150.GA28045@wotan.suse.de/
>
> I'm not sure on how to fix this, flipping the condition would work but
> that is a breaking change and idk if this is ok here.
> Adding a check to also users who have write access to the file would
> remove all the logic here since you would always be allowed to dedup
> FDs you managed to get your hands on.
>
> Any input on this welcome, thx
My guess is that not many users try to dedupe other users' files,
so this feature was never used and nobody complained.
What use case do you think flipping the condition could break?
breaking uapi is not about theoretical use cases and in any
case this needs to be marked with Fixes: and can be backported
as far as anyone who cares wants to backport.
You should add an xfstest for this and include a
_fixed_by_kernel_commit and that will signal all the distros that
care to backport the fix.
Thanks,
Amir.
next prev parent reply other threads:[~2024-05-04 9:43 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-04 4:49 bug in may_dedupe_file allows to deduplicate files we aren't allowed to write to Hugo Valtier
2024-05-04 9:43 ` Amir Goldstein [this message]
2024-05-04 20:50 ` Hugo Valtier
2024-05-05 6:57 ` Amir Goldstein
2024-05-07 22:14 ` Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOQ4uxjh5iQ0_knRebNRS271vR2-2f_9bNZyBG5vUy3rw6xh-g@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=brauner@kernel.org \
--cc=hugo@valtier.fr \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mfasheh@suse.de \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.