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