From: "Darrick J. Wong" <djwong@kernel.org>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Hugo Valtier <hugo@valtier.fr>,
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>
Subject: Re: bug in may_dedupe_file allows to deduplicate files we aren't allowed to write to
Date: Tue, 7 May 2024 15:14:27 -0700 [thread overview]
Message-ID: <20240507221427.GA2049409@frogsfrogsfrogs> (raw)
In-Reply-To: <CAOQ4uxiGpShrki9dnJM1hvz1GPPcDos6P8pAkAz_jksy4gJdsw@mail.gmail.com>
[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@valtier.fr> 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/20180511192651.21324-2-mfasheh@suse.de/
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.
prev parent reply other threads:[~2024-05-07 22:14 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-04 4:49 bug in may_dedupe_file allows to deduplicate files we aren't allowed to write to Hugo Valtier
2024-05-04 9:43 ` Amir Goldstein
2024-05-04 20:50 ` Hugo Valtier
2024-05-05 6:57 ` Amir Goldstein
2024-05-07 22:14 ` Darrick J. Wong [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240507221427.GA2049409@frogsfrogsfrogs \
--to=djwong@kernel.org \
--cc=amir73il@gmail.com \
--cc=brauner@kernel.org \
--cc=hugo@valtier.fr \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark@fasheh.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.