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

[Matthew Wilcox]

On Wed, Dec 07, 2022 at 10:26:54AM +0100, Rasmus Villemoes wrote:
> On 06/12/2022 21.18, 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.
> > 
> > I feel like all the options here are kind of ugly.  Seeing casts in
> > the arguments to container_of should be a red flag!
> > 
> > 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.
> 
> Wait, what? The const-ness or not of dentry is completely irrelevant,
> we're not doing any pointer arithmetic on that to obtain some other
> pointer that morally should have the same const-ness. We're
> dereferencing dentry to get a pointer value, and _that_ pointer value is
> then subject to the pointer arithmetic.

... and this runs up against the fundamental duality of "what does const
mean".  If you declare a region of memory as const, it is read only.
But a pointer to const memory simply means "you may not alter it".
It does not mean "this is read only".  But the compiler doesn't know
that, so of course it warns that you're casting away constness.

> Note that external_name might as well have had the prototype
> 
> static inline struct external_name *external_name(const struct dentry
> *dentry)
> 
> and then your container_of_2 would break.

Fair point.  Actually, it probably should have that prototype since
it doesn't modify dentry.

> I think that if we want to move towards container_of preserving the
> constness of the member pointer, the right fix here is indeed a cast,
> but not inside container_of, but rather to cast away the const afterwards:
> 
>   return (struct external_name *)container_of(dentry->d_name.name,
> struct external_name, name[0]);
> 
> knowing that yes, the dentry only keeps a const pointer to the name[]
> member for good reasons, but the callers very much do need to modify the
> rest of the struct.

I dislike repeating the name of the struct twice.  More than I dislike
container_of_not_const() which gets to reuse the type name.

We could also do ...

	return NOT_CONST(container_of(dentry->d_name.name,
					struct external_name, name[0]);

and have NOT_CONST be the warning sign that we're doing something
unusual.