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*.
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
*without opening the file in write mode which I don't want to do
because it can prevent execution of files which is the exact thing is
5de4480ae7f8 were trying to address in the first place
On Sat, May 4, 2024 at 7:49 AM Hugo Valtier <[email protected]> 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/[email protected]/
>
> 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.
> 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 à 11:43, Amir Goldstein <[email protected]> a écrit :
>
> On Sat, May 4, 2024 at 7:49 AM Hugo Valtier <[email protected]> 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/[email protected]/
>
> >
> > 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.
[change email for Mark Fashe]
On Sat, May 4, 2024 at 11:51 PM Hugo Valtier <[email protected]> wrote:
>
> > 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.
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.
[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 <[email protected]> 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@mail.gmail.com/
Which in turn is discussing the change made here?
https://lore.kernel.org/linux-fsdevel/[email protected]/
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.