Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <CAOQ4uxjssgw1tZrUQvtHHVacSgR9NE0yF8DA3+R5LNFAocCvVQ@mail.gmail.com>
 <000000000000258ac60606589787@google.com> <CAG48ez2UkCR7LMaQfCQVLW4VOZG9CuPDMHG7cBtaAitM=zPBCg@mail.gmail.com>
 <CAG48ez2_XT1XDML756zM2D07BjcJnw22pFiHHrOm-yHvGJHvdw@mail.gmail.com>
In-Reply-To: <CAG48ez2_XT1XDML756zM2D07BjcJnw22pFiHHrOm-yHvGJHvdw@mail.gmail.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Sat, 25 Nov 2023 11:21:27 +0200
Message-ID: <CAOQ4uxj+enOZJiAJaCRnfb1soFS7aonJjHmLXiP3heQAFQoBqg@mail.gmail.com>
Subject: Re: [syzbot] [overlayfs?] KASAN: invalid-free in ovl_copy_up_one
To:     Jann Horn <jannh@google.com>
Cc:     syzbot <syzbot+477d8d8901756d1cbba1@syzkaller.appspotmail.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-unionfs@vger.kernel.org, miklos@szeredi.hu,
        syzkaller-bugs@googlegroups.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Nov 24, 2023 at 5:26=E2=80=AFPM Jann Horn <jannh@google.com> wrote:
>
> On Fri, Nov 24, 2023 at 4:11=E2=80=AFPM Jann Horn <jannh@google.com> wrot=
e:
> >
> > On Wed, Sep 27, 2023 at 5:10=E2=80=AFPM syzbot
> > <syzbot+477d8d8901756d1cbba1@syzkaller.appspotmail.com> wrote:
> > > syzbot has tested the proposed patch and the reproducer did not trigg=
er any issue:
> > >
> > > Reported-and-tested-by: syzbot+477d8d8901756d1cbba1@syzkaller.appspot=
mail.com
> > >
> > > Tested on:
> > >
> > > commit:         8e9b46c4 ovl: do not encode lower fh with upper sb_wr=
i..
> > > git tree:       https://github.com/amir73il/linux.git ovl_want_write
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=3D10d10ffa6=
80000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=3Dbb54ecdfa=
197f132
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=3D477d8d89017=
56d1cbba1
> > > compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils f=
or Debian) 2.40
> >
> > It looks like the fix was submitted without the Reported-by tag, so
> > syzkaller doesn't recognize that the fix has landed... I'll tell
> > syzkaller now which commit the fix is supposed to be in, please
> > correct me if this is wrong:
> >
> > #syz fix: ovl: do not encode lower fh with upper sb_writers held
>
> (Ah, and just for the record: I hadn't realized when writing this that
> the fix was actually in a newer version of the same patch... "git

That is correct.
I am very thankful for syzbot with helping me catch bugs during development
and I would gladly attribute the bot and its owners, but I don't that
Reported-and-tested-by is an adequate tag for a bug that never existed as
far as git history.

Even Tested-by: syzbot could be misleading to stable kernel bots
that may conclude that the patch is a fix that needs to apply to stable.

I am open to suggestions.

Also maybe

#syz correction:

To tell syzbot we are not fixing a bug in upstream, but in a previous
version of a patch that it had tested.

> range-diff 44ef23e481b02df2f17599a24f81cf0045dc5256~1..44ef23e481b02df2f1=
7599a24f81cf0045dc5256
> 5b02bfc1e7e3811c5bf7f0fa626a0694d0dbbd77~1..5b02bfc1e7e3811c5bf7f0fa626a0=
694d0dbbd77"
> shows an added "ovl_get_index_name", I guess that's the fix?)

No, that added ovl_get_index_name() seems like a fluke of the range-diff to=
ol.
All the revisions of this patch always had this same minor change in this l=
ine:

-               err =3D ovl_get_index_name(ofs, c->lowerpath.dentry,
&c->destname);
+               err =3D ovl_get_index_name(ofs, origin, &c->destname);

The fix is obviously in the other part of the range-diff.

Thanks,
Amir.

                if (err)
     -                  return err;
    -+                  goto out;
    ++                  goto out_free_fh;
        } else if (WARN_ON(!c->parent)) {
                /* Disconnected dentry must be copied up to index dir */
     -          return -EIO;
     +          err =3D -EIO;
    -+          goto out;
    ++          goto out_free_fh;
        } else {
                /*
                 * Mark parent "impure" because it may now contain non-pure
    @@ fs/overlayfs/copy_up.c: static int ovl_do_copy_up(struct
ovl_copy_up_ctx *c)
                ovl_end_write(c->dentry);
                if (err)
     -                  return err;
    -+                  goto out;
    ++                  goto out_free_fh;
        }

        /* Should we copyup with O_TMPFILE or with workdir? */
    @@ fs/overlayfs/copy_up.c: static int ovl_do_copy_up(struct
ovl_copy_up_ctx *c)
      out:
        if (to_index)
                kfree(c->destname.name);
    ++out_free_fh:
     +  kfree(fh);
        return err;
      }