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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).