linux-kernel - Re: [PATCH v2 1/4] container_of: add container_of_const() that preserves const-ness of the pointer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y5BO669ZFB3Vp5iX@kroah.com>
Date:   Wed, 7 Dec 2022 09:29:31 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     linux-kernel@...r.kernel.org, Jason Gunthorpe <jgg@...pe.ca>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Jason Gunthorpe <jgg@...dia.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v2 1/4] container_of: add container_of_const() that
 preserves const-ness of the pointer

On Tue, Dec 06, 2022 at 08:18:52PM +0000, Matthew Wilcox wrote:
> On Tue, Dec 06, 2022 at 07:46:47PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Dec 06, 2022 at 05:18:22PM +0000, Matthew Wilcox wrote:
> > >  static inline struct external_name *external_name(struct dentry *dentry)
> > >  {
> > > -	return container_of(dentry->d_name.name, struct external_name, name[0]);
> > > +	return container_of_not_const(dentry->d_name.name,
> > > +				      struct external_name, name[0]);
> > >  }
> > 
> > Will just:
> > 	return container_of((unsigned char *)dentry->d_name.name, struct external_name, name[0]);
> > work by casting away the "const" of the name?
> > 
> > Yeah it's ugly, I never considered the address of a const char * being
> > used as a base to cast back from.  The vfs is fun :)
> 
> Yes, that also works.  This isn't particularly common in the VFS, it's
> just the dcache.  And I understand why it's done like this; you don't
> want rando filesystems modifying dentry names without also updating
> the hash.

Agreed.

> I feel like all the options here are kind of ugly.  Seeing casts in
> the arguments to container_of should be a red flag!

True, but this should be rare, and so they can stand out and they can be
commented about why they are needed, which is good to have.

> Here's a bit of a weird option ...
> 
> +#define container_of_2(ptr, p_m, type, member)                         \
> +       _Generic(ptr,                                                   \
> +               const typeof(*(ptr)) *: (const type *)container_of(ptr->p_m, type, member), \
> +               default: ((type *)container_of(ptr->p_m, type, member)))
> +
> 
>  static inline struct external_name *external_name(struct dentry *dentry)
>  {
> -       return container_of(dentry->d_name.name, struct external_name, name[0]);
> +       return container_of_2(dentry, d_name.name, struct external_name,
> +                               name[0]);
>  }
> 
> so we actually split the first argument into two -- the pointer which
> isn't const, then the pointer member which might be const, but we don't
> use it for the return result of container_of_2.

Ick, that papers over this oddity.  I think it would be better to make
it obvious what is happening here with a cast and a comment instead of
allowing people to use a different macro to "fix it up".  The danger is
of course that this new call would be used when people don't realize it
shouldn't be used.  Let's not even give them that ability please.

thanks,

greg k-h