Re: bug in may_dedupe_file allows to deduplicate files we aren't allowed to write to

[Darrick J. Wong]

[add fsdevel to cc because why not?]

On Sun, May 05, 2024 at 09:57:23AM +0300, Amir Goldstein wrote:
> [change email for Mark Fashe]
> 
> On Sat, May 4, 2024 at 11:51 PM Hugo Valtier <hugo@xxxxxxxxxx> wrote:
> >
> > > My guess is that not many users try to dedupe other users' files,
> > > so this feature was never used and nobody complained.
> >
> > +1

So I guess the rest of the thread is here?

https://lore.kernel.org/lkml/CAF+WW=oKQak6ktiOH75pHSDe7YEkYD-1ditgcsWB=z+aRKJogQ@xxxxxxxxxxxxxx/

Which in turn is discussing the change made here?

https://lore.kernel.org/linux-fsdevel/20180511192651.21324-2-mfasheh@xxxxxxx/

Based on the stated intent in the original patch ("process can write
inode") I do not think Mr. Valtier's patch is correct.
inode_permission(..., MAY_WRITE) returns 0 if the caller can access the
file in the given mode, or some negative errno if it cannot.  I don't
know why he sees the behavior he describes:

"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*."

Which test exactly is the one that results in a denial?  I don't think I
can reproduce this:

$ ls /opt/a /opt/b
-rw-r--r-- 1 root root 65536 May  7 15:09 /opt/a
-rw-rw-rw- 1 root root 65536 May  7 15:09 /opt/b
$ xfs_io -r -c 'dedupe /opt/b 4096 4096 4096' /opt/a
XFS_IOC_FILE_EXTENT_SAME: Operation not permitted

<confused>

> > 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.
> 
> 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
a> 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.

..and yes, the unusual permissions behavior of FIDEDUPERANGE should be
better tested.

--D

> Thanks,
> Amir.