Received-SPF: pass (google.com: domain of linux-kernel+bounces-168902-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <CAF+WW=oKQak6ktiOH75pHSDe7YEkYD-1ditgcsWB=z+aRKJogQ@mail.gmail.com>
 <CAOQ4uxjh5iQ0_knRebNRS271vR2-2f_9bNZyBG5vUy3rw6xh-g@mail.gmail.com> <CAF+WW=rRz0L-P9X2tV9svGdTbhAhpBea=huf-_DDfkz29fXUyQ@mail.gmail.com>
In-Reply-To: <CAF+WW=rRz0L-P9X2tV9svGdTbhAhpBea=huf-_DDfkz29fXUyQ@mail.gmail.com>
From: Amir Goldstein <amir73il@gmail.com>
Date: Sun, 5 May 2024 09:57:23 +0300
Message-ID: <CAOQ4uxiGpShrki9dnJM1hvz1GPPcDos6P8pAkAz_jksy4gJdsw@mail.gmail.com>
Subject: Re: bug in may_dedupe_file allows to deduplicate files we aren't
 allowed to write to
To: Hugo Valtier <hugo@valtier.fr>
Cc: viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Christian Brauner <brauner@kernel.org>, 
	Mark Fasheh <mark@fasheh.com>, "Darrick J. Wong" <djwong@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

[change email for Mark Fashe]

On Sat, May 4, 2024 at 11:51=E2=80=AFPM Hugo Valtier <hugo@valtier.fr> wrot=
e:
>
> > 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 mean=
t:
> > 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.

There is no need to wait for the 6.9 release.
Fixes can and should be posted at any time.

> Once that is merged in some tree (fsdevel I guess ?) I submit a patch for

Yes, this is a good candidate for Christian Brauner's vfs tree.
Please CC the VFS maintainers (from MAINTAINERS file) and fsdevel.

A note about backporting to stable kernels.
stable maintainer bots would do best effort to auto backport
patches marked with a Fixes: commit to the supported LTS kernel,
once the fix is merged to master,
but if the fix does not apply cleanly, you will need to post the
backport yourself (if you want the fix backported).

For your case, the fix will not apply cleanly before
4609e1f18e19 ("fs: port ->permission() to pass mnt_idmap")
so at lease from 6.1.y and backwards, you will need to post
manual backports if you want the fix in LTS kernels or you can
let the distros that find the new xfstest failure take care of that...

> xfstest which adds a regression test and has _fixed_by_kernel_commit
> mentioning the commit just merged in the fsdevel linux tree.

Correct.
You may take inspiration from existing dedupe tests
[CC Darrick who wrote most of them]
but I did not find any test coverage for may_dedupe_file() among them.

There is one test that is dealing with permissions that you can
use as a template:

$ git grep -w _begin_fstest.*dedupe tests/generic/|grep perms
tests/generic/674:_begin_fstest auto clone quick perms dedupe

Hint: use $XFS_IO_PROG -r to open the destination file read only.

Because there is currently no test coverage for read-only dest
for the admin and user owned files, I suggest that you start with
writing this test, making sure that your fix does not regress it and
then add the other writable file case.

Thanks,
Amir.