Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
Date:   Sat, 25 Nov 2023 01:21:39 -0800
In-Reply-To: <CAOQ4uxj+enOZJiAJaCRnfb1soFS7aonJjHmLXiP3heQAFQoBqg@mail.gmail.com>
Message-ID: <000000000000ab2787060af698fc@google.com>
Subject: Re: [syzbot] [overlayfs?] KASAN: invalid-free in ovl_copy_up_one
From:   syzbot <syzbot+477d8d8901756d1cbba1@syzkaller.appspotmail.com>
To:     amir73il@gmail.com
Cc:     amir73il@gmail.com, jannh@google.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: 8BIT
Precedence: bulk

> On Fri, Nov 24, 2023 at 5:26 PM Jann Horn <jannh@google.com> wrote:
>>
>> On Fri, Nov 24, 2023 at 4:11 PM Jann Horn <jannh@google.com> wrote:
>> >
>> > On Wed, Sep 27, 2023 at 5:10 PM syzbot
>> > <syzbot+477d8d8901756d1cbba1@syzkaller.appspotmail.com> wrote:
>> > > syzbot has tested the proposed patch and the reproducer did not trigger any issue:
>> > >
>> > > Reported-and-tested-by: syzbot+477d8d8901756d1cbba1@syzkaller.appspotmail.com
>> > >
>> > > Tested on:
>> > >
>> > > commit:         8e9b46c4 ovl: do not encode lower fh with upper sb_wri..
>> > > git tree:       https://github.com/amir73il/linux.git ovl_want_write
>> > > console output: https://syzkaller.appspot.com/x/log.txt?x=10d10ffa680000
>> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=bb54ecdfa197f132
>> > > dashboard link: https://syzkaller.appspot.com/bug?extid=477d8d8901756d1cbba1
>> > > compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for 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:

unknown command "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..44ef23e481b02df2f17599a24f81cf0045dc5256
>> 5b02bfc1e7e3811c5bf7f0fa626a0694d0dbbd77~1..5b02bfc1e7e3811c5bf7f0fa626a0694d0dbbd77"
>> 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 tool.
> All the revisions of this patch always had this same minor change in this line:
>
> -               err = ovl_get_index_name(ofs, c->lowerpath.dentry,
> &c->destname);
> +               err = 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 = -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;
>       }