Hi, this is your Linux kernel regression tracker speaking.
I noticed a regression report in bugzilla.kernel.org. As many (most?)
kernel developer don't keep an eye on it, I decided to forward it by
mail. Quoting from https://bugzilla.kernel.org/show_bug.cgi?id=216738 :
> Pierre Labastie 2022-11-24 14:53:33 UTC
>
> Created attachment 303287 [details]
> C program for reproducing the bug
>
> Not sure this is the right place to report this, but at least the offending commit
[offending commit is 164f4064ca8 ("keep iocb_flags() result cached in
struct file"), as specified in the "Kernel Version:" field in bugzilla]
> is in this component...
>
> Steps to reproduce:
> $ gcc repro.c
> $ rm -f toto
> $ ./a.out
> $ cat toto; echo
>
> On an ext4 fs, the output is (on all versions):
> abcdefghijklmnopqr
>
> Now, make an overlayfs:
> $ mkdir -p up lo wo mnt
> $ sudo mount -t overlay overlay -oupperdir=up,lowerdir=lo,workdir=wo mnt
> $ cd mnt
> $ rm f toto
> $ ../a.out
> $ cat toto; echo
>
> before the said commit, the output is:
> abcdefghijklmnopqr
>
> after the said commit, the output is:
> ghijklmnopqr
>
> That is the file is truncated when opened with O_RDWR, with O_APPEND added later, but not when opened with both.
See the ticket for more details.
BTW, let me use this mail to also add the report to the list of tracked
regressions to ensure it's doesn't fall through the cracks:
#regzbot introduced: 164f4064ca8
https://bugzilla.kernel.org/show_bug.cgi?id=216738
#regzbot ignore-activity
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
P.S.: As the Linux kernel's regression tracker I deal with a lot of
reports and sometimes miss something important when writing mails like
this. If that's the case here, don't hesitate to tell me in a public
reply, it's in everyone's interest to set the public record straight.
On Thu, Nov 24, 2022 at 04:47:56PM +0100, Thorsten Leemhuis wrote:
> Hi, this is your Linux kernel regression tracker speaking.
>
> I noticed a regression report in bugzilla.kernel.org. As many (most?)
> kernel developer don't keep an eye on it, I decided to forward it by
> mail. Quoting from https://bugzilla.kernel.org/show_bug.cgi?id=216738 :
>
> > Pierre Labastie 2022-11-24 14:53:33 UTC
> >
> > Created attachment 303287 [details]
> > C program for reproducing the bug
> >
> > Not sure this is the right place to report this, but at least the offending commit
>
> [offending commit is 164f4064ca8 ("keep iocb_flags() result cached in
> struct file"), as specified in the "Kernel Version:" field in bugzilla]
So basically we have this
static int ovl_change_flags(struct file *file, unsigned int flags)
{
struct inode *inode = file_inode(file);
int err;
flags &= OVL_SETFL_MASK;
if (((flags ^ file->f_flags) & O_APPEND) && IS_APPEND(inode))
return -EPERM;
if ((flags & O_DIRECT) && !(file->f_mode & FMODE_CAN_ODIRECT))
return -EINVAL;
if (file->f_op->check_flags) {
err = file->f_op->check_flags(flags);
if (err)
return err;
}
spin_lock(&file->f_lock);
file->f_flags = (file->f_flags & ~OVL_SETFL_MASK) | flags;
spin_unlock(&file->f_lock);
return 0;
}
open-coding what setfl() would've done, without updating ->f_iocb_flags...
Not hard to deal with...
I could pick it in vfs.git #fixes, or Miklos could put it through his tree.
Miklos, which way would you prefer that to go?
[PATCH] update ->f_iocb_flags when ovl_change_flags() modifies ->f_flags
ovl_change_flags() is an open-coded variant of fs/fcntl.c:setfl() and it got
missed by 164f4064ca81e "keep iocb_flags() result cached in struct file";
the same change applies there.
Reported-by: Pierre Labastie <[email protected]>
Fixes: 164f4064ca81e "keep iocb_flags() result cached in struct file"
Signed-off-by: Al Viro <[email protected]>
---
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index a1a22f58ba18..dd688a842b0b 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -96,6 +96,7 @@ static int ovl_change_flags(struct file *file, unsigned int flags)
spin_lock(&file->f_lock);
file->f_flags = (file->f_flags & ~OVL_SETFL_MASK) | flags;
+ file->f_iocb_flags = iocb_flags(file);
spin_unlock(&file->f_lock);
return 0;
On Thu, 24 Nov 2022 at 18:03, Al Viro <[email protected]> wrote:
> So basically we have this
> static int ovl_change_flags(struct file *file, unsigned int flags)
> {
> struct inode *inode = file_inode(file);
> int err;
>
> flags &= OVL_SETFL_MASK;
>
> if (((flags ^ file->f_flags) & O_APPEND) && IS_APPEND(inode))
> return -EPERM;
>
> if ((flags & O_DIRECT) && !(file->f_mode & FMODE_CAN_ODIRECT))
> return -EINVAL;
>
> if (file->f_op->check_flags) {
> err = file->f_op->check_flags(flags);
> if (err)
> return err;
> }
>
> spin_lock(&file->f_lock);
> file->f_flags = (file->f_flags & ~OVL_SETFL_MASK) | flags;
> spin_unlock(&file->f_lock);
>
> return 0;
> }
> open-coding what setfl() would've done, without updating ->f_iocb_flags...
> Not hard to deal with...
>
> I could pick it in vfs.git #fixes, or Miklos could put it through his tree.
> Miklos, which way would you prefer that to go?
I'll pick this into #overlayfs-next, as a PR for this cycle is needed anyway.
Thanks,
Miklos
On Thu, Nov 24, 2022 at 09:33:54PM +0100, Miklos Szeredi wrote:
> > I could pick it in vfs.git #fixes, or Miklos could put it through his tree.
> > Miklos, which way would you prefer that to go?
>
> I'll pick this into #overlayfs-next, as a PR for this cycle is needed anyway.
OK... FWIW, I wonder if exposing setfl() would and using it instead of
open-coding would be a good idea - these two are very close to each other...
On 24.11.22 18:03, Al Viro wrote:
> On Thu, Nov 24, 2022 at 04:47:56PM +0100, Thorsten Leemhuis wrote:
> [...]
Al: thx for fixing this!
> I could pick it in vfs.git #fixes, or Miklos could put it through his tree.
> Miklos, which way would you prefer that to go?
>
> [PATCH] update ->f_iocb_flags when ovl_change_flags() modifies ->f_flags
>
> ovl_change_flags() is an open-coded variant of fs/fcntl.c:setfl() and it got
> missed by 164f4064ca81e "keep iocb_flags() result cached in struct file";
> the same change applies there.
>
> Reported-by: Pierre Labastie <[email protected]>
Miklos, if you pick this up, could you for the sake of future code
archeologists please add this here:
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216738
tia! To explain: Linus[1] and others considered proper link tags in
cases like this important, as they allow anyone to look into the
backstory weeks or years from now. That why they should be placed here,
as outlined by the documentation[2]. I care personally, because these
tags make my regression tracking efforts a whole lot easier, as they
allow my tracking bot 'regzbot' to automatically connect reports with
patches posted or committed to fix tracked regressions.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
P.S.: As the Linux kernel's regression tracker I deal with a lot of
reports and sometimes miss something important when writing mails like
this. If that's the case here, don't hesitate to tell me in a public
reply, it's in everyone's interest to set the public record straight.
[1] for details, see:
https://lore.kernel.org/all/CAHk-=wjMmSZzMJ3Xnskdg4+GGz=5p5p+GSYyFBTh0f-DgvdBWg@mail.gmail.com/
https://lore.kernel.org/all/CAHk-=wgs38ZrfPvy=nOwVkVzjpM3VFU1zobP37Fwd_h9iAD5JQ@mail.gmail.com/
https://lore.kernel.org/all/CAHk-=wjxzafG-=J8oT30s7upn4RhBs6TX-uVFZ5rME+L5_DoJA@mail.gmail.com/
[2] see Documentation/process/submitting-patches.rst
(http://docs.kernel.org/process/submitting-patches.html) and
Documentation/process/5.Posting.rst
(https://docs.kernel.org/process/5.Posting.html)