Received-SPF: pass (google.com: domain of linux-kernel+bounces-168818-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <CAF+WW=oKQak6ktiOH75pHSDe7YEkYD-1ditgcsWB=z+aRKJogQ@mail.gmail.com>
 <CAOQ4uxjh5iQ0_knRebNRS271vR2-2f_9bNZyBG5vUy3rw6xh-g@mail.gmail.com>
In-Reply-To: <CAOQ4uxjh5iQ0_knRebNRS271vR2-2f_9bNZyBG5vUy3rw6xh-g@mail.gmail.com>
From: Hugo Valtier <hugo@valtier.fr>
Date: Sat, 4 May 2024 22:50:35 +0200
Message-ID: <CAF+WW=rRz0L-P9X2tV9svGdTbhAhpBea=huf-_DDfkz29fXUyQ@mail.gmail.com>
Subject: Re: bug in may_dedupe_file allows to deduplicate files we aren't
 allowed to write to
To: Amir Goldstein <amir73il@gmail.com>
Cc: mfasheh@suse.de, viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Christian Brauner <brauner@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

> My guess is that not many users try to dedupe other users' files,
> so this feature was never used and nobody complained.

+1

Thx for the answer, I'm new to this to be sure I understood what you meant:
> 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.

So right now I wait for 6.9 to be released soon enough then
I then submit my patch which invert the condition.
Once that is merged in some tree (fsdevel I guess ?) I submit a patch for
xfstest which adds a regression test and has _fixed_by_kernel_commit
mentioning the commit just merged in the fsdevel linux tree.

Le sam. 4 mai 2024 =C3=A0 11:43, Amir Goldstein <amir73il@gmail.com> a =C3=
=A9crit :
>
> On Sat, May 4, 2024 at 7:49=E2=80=AFAM Hugo Valtier <hugo@valtier.fr> wro=
te:
> >
> > 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.d=
e/
>
> >
> > 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.