Re: [PATCH v2] ovl: keep err zero after successful ovl_cache_get()

[Nirmoy Das]

Hi Amir,

On 15.05.26 19:39, Amir Goldstein wrote:
> On Fri, May 15, 2026 at 1:16 PM Nirmoy Das <nirmoyd@xxxxxxxxxx> wrote:
> > Hi Amir,
> >
> > On 14.05.26 22:19, Amir Goldstein wrote:
> > > On Thu, May 14, 2026 at 5:26 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > > > On Thu, May 14, 2026 at 4:43 PM Nirmoy Das <nirmoyd@xxxxxxxxxx> wrote:
> > > > > ovl_iterate_merged() stores PTR_ERR(cache) in err before checking
> > > > > IS_ERR(cache). On success err holds the truncated cache pointer and
> > > > > can be returned as a bogus non-zero error.
> > > > >
> > > > > The syzbot reproducer reaches this through overlay-on-overlay readdir:
> > > > >
> > > > >     getdents64
> > > > >       iterate_dir(outer overlay file)
> > > > >         ovl_iterate_merged()
> > > > >           ovl_cache_get()
> > > > >             ovl_dir_read_merged()
> > > > >               ovl_dir_read()
> > > > >                 iterate_dir(inner overlay file)
> > > > >                   ovl_iterate_merged()
> > > > >
> > > > > Only compute PTR_ERR(cache) on the error path.
> > > > >
> > > > > Fixes: d25e4b739f83 ("ovl: refactor ovl_iterate() and port to cred guard")
> > > > > Reported-by: syzbot+a16fb0cce329a320661c@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > > Closes: https://syzkaller.appspot.com/bug?extid=a16fb0cce329a320661c
> > > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > > Signed-off-by: Nirmoy Das <nirmoyd@xxxxxxxxxx>
> > > > > ---
> > > > > v2:
> > > > >    - Drop the now-redundant 'int err = 0' initializer and the trailing
> > > > >      'return err' in ovl_iterate_merged(); err is only used inside the
> > > > >      loop's update-check, so the function can just return 0 on success.
> > > > >      (Amir Goldstein)
> > > > >    - Link to v1:
> > > > >      https://lore.kernel.org/all/20260514111354.3552538-1-nirmoyd@xxxxxxxxxx/
> > > > I queue this up and will work on fortifying patches.
> > > Nirmoy,
> > >
> > > I pushed fortify patches to ovl-fixes on my github [1].
> > >
> > > Can you verify that the assertions trigger if you revert your fix
> > > and run the reproducer?
> > >
> > > I imagine they would trigger much more frequently than the KASAN
> > > warnings do.
> >
> > Yes, the assertion triggers with your ovl-fixes branch after reverting
> > my fix.
> >
> > 9541f25af774 Revert "ovl: keep err zero after successful ovl_cache_get()"
> > 1c067d912e47 ovl: add assertions in dir cache code
> > 98e3a2d258e9 ovl: fix race between copy-up and open of a directory
> > 4f80bb375112 ovl: keep err zero after successful ovl_cache_get()
> > 18de6460b6bd ovl: opt-in for fortified ERR_PTR()
> > 690bd87e1fef err_ptr.h: introduce ERR_PTR_SAFE()
> > 7fd2df204f34 Linux 7.1-rc2
> >
> > Running the syz reproducer with panic_on_warn=1 triggered:
> >
> > [   55.404636] ------------[ cut here ]------------
> > [   55.404646] WARNING: fs/overlayfs/readdir.c:511 at
> > ovl_iterate+0x4c0/0x5bc, CPU#2: syz-ovl-iterate/14575
> > [   55.406875] CPU: 2 UID: 0 PID: 14575 Comm: syz-ovl-iterate Not
> > tainted 7.1.0-rc2-g9541f25af774 #1 PREEMPT
> > [   55.408328] pc : ovl_iterate+0x4c0/0x5bc
> > [   55.408632] lr : ovl_iterate+0x4b4/0x5bc
> > [   55.413504] x2 : 0000000000000000 x1 : 0000000000000000 x0 :
> > ffffffffc152db40
> > [   55.414036] Call trace:
> > [   55.414209]  ovl_iterate+0x4c0/0x5bc (P)
> > [   55.414503]  wrap_directory_iterator+0x60/0x90
> > [   55.414809]  shared_ovl_iterate+0x18/0x24
> > [   55.415125]  iterate_dir+0x10c/0x3a4
> > [   55.415365]  __arm64_sys_getdents64+0xe0/0x1e4
> > [   55.417312] Kernel panic - not syncing: kernel: panic_on_warn set ...
> Thanks for testing!
>
> Did it trigger faster than the KASAN warning?
> I'd imagine that it would?


I lost my setup. I think it was quicker but I don't conclusive data. I 
will update you next week.


Regards,

Nirmoy

> Thanks,
> Amir.