On Fri, Jun 09, 2023 at 12:09:08PM -0400, Colin Walters wrote:
> Cool work. It will be interesting to do some performance testing on what does it actually look like to create ~500 or whatever overlayfs layers now that we can.
>
> On Fri, Jun 9, 2023, at 11:41 AM, Christian Brauner wrote:
> >
> > +static int ovl_init_fs_context(struct fs_context *fc)
> > +{
> > + struct ovl_fs_context *ctx = NULL;
> > + struct ovl_fs *ofs = NULL;
> > +
> > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL_ACCOUNT);
> > + if (!ctx)
> > + goto out_err;
>
> It looks to me like in this case, ofs will be NULL, then:
>
> > +out_err:
> > + ovl_fs_context_free(ctx);
> > + ovl_free_fs(ofs);
>
> And then we'll jump here and `ovl_free_fs` is not NULL safe.
>
> I think the previous code was correct here as it just jumped directly to "out:".
Good catch, thanks!
>
>
> (I've always wondered why there's no usage of __attribute__((cleanup))
> in kernel code and in our userspace code doing that we have the free
> functions be no-ops on NULL which systematically avoids these bugs,
> but then again maybe the real fix is Rust ;) )
I have talked about this before and actually do have a PoC patch in my
tree somewhere I never bothered to send it because it didn't feel like I
had the time for the flamewar+bikeshed that would end up
happening... Maybe I should just try.
