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

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

[Greg Kroah-Hartman]

container_of does not preserve the const-ness of a pointer that is
passed into it, which can cause C code that passes in a const pointer to
get a pointer back that is not const and then scribble all over the data
in it.  To prevent this, container_of_const() will preserve the const
status of the pointer passed into it using the newly available _Generic()
method.

Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2: - removed one parameter, now matches container_of(), thanks to
      suggestion from Sakari
    - changed Jason's tag to suggested-by and reviewed-by
    - added Andy's tag

 include/linux/container_of.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/container_of.h b/include/linux/container_of.h
index 2008e9f4058c..1d898f9158b4 100644
--- a/include/linux/container_of.h
+++ b/include/linux/container_of.h
@@ -22,4 +22,17 @@
 		      "pointer type mismatch in container_of()");	\
 	((type *)(__mptr - offsetof(type, member))); })
 
+/**
+ * container_of_const - cast a member of a structure out to the containing
+ *			structure and preserve the const-ness of the pointer
+ * @ptr:		the pointer to the member
+ * @type:		the type of the container struct this is embedded in.
+ * @member:		the name of the member within the struct.
+ */
+#define container_of_const(ptr, type, member)				\
+	_Generic(ptr,							\
+		const typeof(*(ptr)) *: ((const type *)container_of(ptr, type, member)),\
+		default: ((type *)container_of(ptr, type, member))	\
+	)
+
 #endif	/* _LINUX_CONTAINER_OF_H */
-- 
2.38.1

[PATCH v2 2/4] device.h: move kobj_to_dev() to use container_of_const()

[Greg Kroah-Hartman]

Instead of rolling our own const-checking logic, use the newly
introduced container_of_const() to handle it all for us automatically.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2: - respin with changed container_of_const() parameters

 include/linux/device.h | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index 84ae52de6746..8d172d06b8c1 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -680,26 +680,7 @@ struct device_link {
 	bool supplier_preactivated; /* Owned by consumer probe. */
 };
 
-static inline struct device *__kobj_to_dev(struct kobject *kobj)
-{
-	return container_of(kobj, struct device, kobj);
-}
-
-static inline const struct device *__kobj_to_dev_const(const struct kobject *kobj)
-{
-	return container_of(kobj, const struct device, kobj);
-}
-
-/*
- * container_of() will happily take a const * and spit back a non-const * as it
- * is just doing pointer math.  But we want to be a bit more careful in the
- * driver code, so manually force any const * of a kobject to also be a const *
- * to a device.
- */
-#define kobj_to_dev(kobj)					\
-	_Generic((kobj),					\
-		 const struct kobject *: __kobj_to_dev_const,	\
-		 struct kobject *: __kobj_to_dev)(kobj)
+#define kobj_to_dev(__kobj)	container_of_const(__kobj, struct device, kobj)
 
 /**
  * device_iommu_mapped - Returns true when the device DMA is translated
-- 
2.38.1

Re: [PATCH v2 2/4] device.h: move kobj_to_dev() to use container_of_const()

[Rafael J. Wysocki]

On Mon, Dec 5, 2022 at 1:12 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Instead of rolling our own const-checking logic, use the newly
> introduced container_of_const() to handle it all for us automatically.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
> v2: - respin with changed container_of_const() parameters
>
>  include/linux/device.h | 21 +--------------------
>  1 file changed, 1 insertion(+), 20 deletions(-)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 84ae52de6746..8d172d06b8c1 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -680,26 +680,7 @@ struct device_link {
>         bool supplier_preactivated; /* Owned by consumer probe. */
>  };
>
> -static inline struct device *__kobj_to_dev(struct kobject *kobj)
> -{
> -       return container_of(kobj, struct device, kobj);
> -}
> -
> -static inline const struct device *__kobj_to_dev_const(const struct kobject *kobj)
> -{
> -       return container_of(kobj, const struct device, kobj);
> -}
> -
> -/*
> - * container_of() will happily take a const * and spit back a non-const * as it
> - * is just doing pointer math.  But we want to be a bit more careful in the
> - * driver code, so manually force any const * of a kobject to also be a const *
> - * to a device.
> - */
> -#define kobj_to_dev(kobj)                                      \
> -       _Generic((kobj),                                        \
> -                const struct kobject *: __kobj_to_dev_const,   \
> -                struct kobject *: __kobj_to_dev)(kobj)
> +#define kobj_to_dev(__kobj)    container_of_const(__kobj, struct device, kobj)
>
>  /**
>   * device_iommu_mapped - Returns true when the device DMA is translated
> --
> 2.38.1
>

[PATCH v2 3/4] usb.h: take advantage of container_of_const()

[Greg Kroah-Hartman]

Instead of rolling our own const-checking logic in to_usb_interface()
and to_usb_device() use the newly added container_of_const() instead,
making the logic much simpler overall.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2: - respin with changed container_of_const() parameters

 include/linux/usb.h | 42 ++----------------------------------------
 1 file changed, 2 insertions(+), 40 deletions(-)

diff --git a/include/linux/usb.h b/include/linux/usb.h
index 4b463a5e4ba2..010c681b8822 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -259,26 +259,7 @@ struct usb_interface {
 	struct work_struct reset_ws;	/* for resets in atomic context */
 };
 
-static inline struct usb_interface *__to_usb_interface(struct device *d)
-{
-	return container_of(d, struct usb_interface, dev);
-}
-
-static inline const struct usb_interface *__to_usb_interface_const(const struct device *d)
-{
-	return container_of(d, struct usb_interface, dev);
-}
-
-/*
- * container_of() will happily take a const * and spit back a non-const * as it
- * is just doing pointer math.  But we want to be a bit more careful in the USB
- * driver code, so manually force any const * of a device to also be a const *
- * to a usb_device.
- */
-#define to_usb_interface(dev)						\
-	_Generic((dev),							\
-		 const struct device *: __to_usb_interface_const,	\
-		 struct device *: __to_usb_interface)(dev)
+#define to_usb_interface(__dev)	container_of_const(__dev, struct usb_interface, dev)
 
 static inline void *usb_get_intfdata(struct usb_interface *intf)
 {
@@ -730,26 +711,7 @@ struct usb_device {
 	unsigned use_generic_driver:1;
 };
 
-static inline struct usb_device *__to_usb_device(struct device *d)
-{
-	return container_of(d, struct usb_device, dev);
-}
-
-static inline const struct usb_device *__to_usb_device_const(const struct device *d)
-{
-	return container_of(d, struct usb_device, dev);
-}
-
-/*
- * container_of() will happily take a const * and spit back a non-const * as it
- * is just doing pointer math.  But we want to be a bit more careful in the USB
- * driver code, so manually force any const * of a device to also be a const *
- * to a usb_device.
- */
-#define to_usb_device(dev)					\
-	_Generic((dev),						\
-		 const struct device *: __to_usb_device_const,	\
-		 struct device *: __to_usb_device)(dev)
+#define to_usb_device(__dev)	container_of_const(__dev, struct usb_device, dev)
 
 static inline struct usb_device *__intf_to_usbdev(struct usb_interface *intf)
 {
-- 
2.38.1

[PATCH v2 4/4] firmware_loader: fix up to_fw_sysfs() to preserve const

[Greg Kroah-Hartman]

to_fw_sysfs() was changed in commit 23680f0b7d7f ("driver core: make
struct class.dev_uevent() take a const *") to pass in a const pointer
but not pass it back out to handle some changes in the driver core.
That isn't the best idea as it could cause problems if used incorrectly,
so switch to use the container_of_const() macro instead which will
preserve the const status of the pointer and enforce it by the compiler.

Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Acked-by: Russ Weight <russell.h.weight@intel.com>
Fixes: 23680f0b7d7f ("driver core: make struct class.dev_uevent() take a const *")
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2: - respin with changed container_of_const() parameters

 drivers/base/firmware_loader/sysfs.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/base/firmware_loader/sysfs.h b/drivers/base/firmware_loader/sysfs.h
index fd0b4ad9bdbb..2060add8ef81 100644
--- a/drivers/base/firmware_loader/sysfs.h
+++ b/drivers/base/firmware_loader/sysfs.h
@@ -80,11 +80,7 @@ struct fw_sysfs {
 	struct firmware *fw;
 	void *fw_upload_priv;
 };
-
-static inline struct fw_sysfs *to_fw_sysfs(const struct device *dev)
-{
-	return container_of(dev, struct fw_sysfs, dev);
-}
+#define to_fw_sysfs(__dev)	container_of_const(__dev, struct fw_sysfs, dev)
 
 void __fw_load_abort(struct fw_priv *fw_priv);
 
-- 
2.38.1

Re: [PATCH v2 4/4] firmware_loader: fix up to_fw_sysfs() to preserve const

[Rafael J. Wysocki]

On Mon, Dec 5, 2022 at 1:12 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> to_fw_sysfs() was changed in commit 23680f0b7d7f ("driver core: make
> struct class.dev_uevent() take a const *") to pass in a const pointer
> but not pass it back out to handle some changes in the driver core.
> That isn't the best idea as it could cause problems if used incorrectly,
> so switch to use the container_of_const() macro instead which will
> preserve the const status of the pointer and enforce it by the compiler.
>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Acked-by: Russ Weight <russell.h.weight@intel.com>
> Fixes: 23680f0b7d7f ("driver core: make struct class.dev_uevent() take a const *")
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
> v2: - respin with changed container_of_const() parameters
>
>  drivers/base/firmware_loader/sysfs.h | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/sysfs.h b/drivers/base/firmware_loader/sysfs.h
> index fd0b4ad9bdbb..2060add8ef81 100644
> --- a/drivers/base/firmware_loader/sysfs.h
> +++ b/drivers/base/firmware_loader/sysfs.h
> @@ -80,11 +80,7 @@ struct fw_sysfs {
>         struct firmware *fw;
>         void *fw_upload_priv;
>  };
> -
> -static inline struct fw_sysfs *to_fw_sysfs(const struct device *dev)
> -{
> -       return container_of(dev, struct fw_sysfs, dev);
> -}
> +#define to_fw_sysfs(__dev)     container_of_const(__dev, struct fw_sysfs, dev)
>
>  void __fw_load_abort(struct fw_priv *fw_priv);
>
> --
> 2.38.1
>

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

[Rafael J. Wysocki]

On Mon, Dec 5, 2022 at 1:12 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> container_of does not preserve the const-ness of a pointer that is
> passed into it, which can cause C code that passes in a const pointer to
> get a pointer back that is not const and then scribble all over the data
> in it.  To prevent this, container_of_const() will preserve the const
> status of the pointer passed into it using the newly available _Generic()
> method.
>
> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
> v2: - removed one parameter, now matches container_of(), thanks to
>       suggestion from Sakari
>     - changed Jason's tag to suggested-by and reviewed-by
>     - added Andy's tag
>
>  include/linux/container_of.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/container_of.h b/include/linux/container_of.h
> index 2008e9f4058c..1d898f9158b4 100644
> --- a/include/linux/container_of.h
> +++ b/include/linux/container_of.h
> @@ -22,4 +22,17 @@
>                       "pointer type mismatch in container_of()");       \
>         ((type *)(__mptr - offsetof(type, member))); })
>
> +/**
> + * container_of_const - cast a member of a structure out to the containing
> + *                     structure and preserve the const-ness of the pointer
> + * @ptr:               the pointer to the member
> + * @type:              the type of the container struct this is embedded in.
> + * @member:            the name of the member within the struct.
> + */
> +#define container_of_const(ptr, type, member)                          \
> +       _Generic(ptr,                                                   \
> +               const typeof(*(ptr)) *: ((const type *)container_of(ptr, type, member)),\
> +               default: ((type *)container_of(ptr, type, member))      \
> +       )
> +
>  #endif /* _LINUX_CONTAINER_OF_H */
> --
> 2.38.1
>

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

[Sakari Ailus]

Hi Greg,

On Mon, Dec 05, 2022 at 01:12:03PM +0100, Greg Kroah-Hartman wrote:
> container_of does not preserve the const-ness of a pointer that is
> passed into it, which can cause C code that passes in a const pointer to
> get a pointer back that is not const and then scribble all over the data
> in it.  To prevent this, container_of_const() will preserve the const
> status of the pointer passed into it using the newly available _Generic()
> method.
> 
> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v2: - removed one parameter, now matches container_of(), thanks to
>       suggestion from Sakari
>     - changed Jason's tag to suggested-by and reviewed-by
>     - added Andy's tag

Thanks for the update. For the set:

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> 
>  include/linux/container_of.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/container_of.h b/include/linux/container_of.h
> index 2008e9f4058c..1d898f9158b4 100644
> --- a/include/linux/container_of.h
> +++ b/include/linux/container_of.h
> @@ -22,4 +22,17 @@
>  		      "pointer type mismatch in container_of()");	\
>  	((type *)(__mptr - offsetof(type, member))); })
>  
> +/**
> + * container_of_const - cast a member of a structure out to the containing
> + *			structure and preserve the const-ness of the pointer
> + * @ptr:		the pointer to the member
> + * @type:		the type of the container struct this is embedded in.
> + * @member:		the name of the member within the struct.
> + */
> +#define container_of_const(ptr, type, member)				\
> +	_Generic(ptr,							\
> +		const typeof(*(ptr)) *: ((const type *)container_of(ptr, type, member)),\
> +		default: ((type *)container_of(ptr, type, member))	\
> +	)
> +
>  #endif	/* _LINUX_CONTAINER_OF_H */

-- 
Kind regards,

Sakari Ailus

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

[Matthew Wilcox]

On Mon, Dec 05, 2022 at 01:12:03PM +0100, Greg Kroah-Hartman wrote:
> +/**
> + * container_of_const - cast a member of a structure out to the containing
> + *			structure and preserve the const-ness of the pointer
> + * @ptr:		the pointer to the member
> + * @type:		the type of the container struct this is embedded in.
> + * @member:		the name of the member within the struct.
> + */
> +#define container_of_const(ptr, type, member)				\
> +	_Generic(ptr,							\
> +		const typeof(*(ptr)) *: ((const type *)container_of(ptr, type, member)),\
> +		default: ((type *)container_of(ptr, type, member))	\
> +	)
> +

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

I tried doing this:

+++ b/include/linux/container_of.h
@@ -15,11 +15,17 @@
  *
  * WARNING: any const qualifier of @ptr is lost.
  */
-#define container_of(ptr, type, member) ({                             \
+#define _c_of(ptr, type, member) ({                                    \
        void *__mptr = (void *)(ptr);                                   \
        static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
                      __same_type(*(ptr), void),                        \
                      "pointer type mismatch in container_of()");       \
        ((type *)(__mptr - offsetof(type, member))); })

+#define container_of(ptr, type, m)                                     \
+       _Generic(ptr,                                                   \
+               const typeof(*(ptr)) *: (const type *)_c_of(ptr, type, m),\
+               default: ((type *)_c_of(ptr, type, m))                  \
+       )
+
 #endif /* _LINUX_CONTAINER_OF_H */

(whitespace damaged, yes the kernel-doc is now in the wrong place, etc)

It found a few problems; just building the mlx5 driver (I happened to be
doing some work on it in that tree).  We're definitely not ready to do
that yet, but I'll send a few patches to prepare for it.

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

[Greg Kroah-Hartman]

On Tue, Dec 06, 2022 at 03:13:29PM +0000, Matthew Wilcox wrote:
> On Mon, Dec 05, 2022 at 01:12:03PM +0100, Greg Kroah-Hartman wrote:
> > +/**
> > + * container_of_const - cast a member of a structure out to the containing
> > + *			structure and preserve the const-ness of the pointer
> > + * @ptr:		the pointer to the member
> > + * @type:		the type of the container struct this is embedded in.
> > + * @member:		the name of the member within the struct.
> > + */
> > +#define container_of_const(ptr, type, member)				\
> > +	_Generic(ptr,							\
> > +		const typeof(*(ptr)) *: ((const type *)container_of(ptr, type, member)),\
> > +		default: ((type *)container_of(ptr, type, member))	\
> > +	)
> > +
> 
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> I tried doing this:
> 
> +++ b/include/linux/container_of.h
> @@ -15,11 +15,17 @@
>   *
>   * WARNING: any const qualifier of @ptr is lost.
>   */
> -#define container_of(ptr, type, member) ({                             \
> +#define _c_of(ptr, type, member) ({                                    \
>         void *__mptr = (void *)(ptr);                                   \
>         static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
>                       __same_type(*(ptr), void),                        \
>                       "pointer type mismatch in container_of()");       \
>         ((type *)(__mptr - offsetof(type, member))); })
> 
> +#define container_of(ptr, type, m)                                     \
> +       _Generic(ptr,                                                   \
> +               const typeof(*(ptr)) *: (const type *)_c_of(ptr, type, m),\
> +               default: ((type *)_c_of(ptr, type, m))                  \
> +       )
> +
>  #endif /* _LINUX_CONTAINER_OF_H */
> 
> (whitespace damaged, yes the kernel-doc is now in the wrong place, etc)
> 
> It found a few problems; just building the mlx5 driver (I happened to be
> doing some work on it in that tree).  We're definitely not ready to do
> that yet, but I'll send a few patches to prepare for it.

Yeah, that's a good long-term goal to have here.  Once everything is
moved over, switching all container_of_const() to just container_of()
should be simple.

thanks,

greg k-h

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

[Matthew Wilcox]

On Tue, Dec 06, 2022 at 04:54:45PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 06, 2022 at 03:13:29PM +0000, Matthew Wilcox wrote:
> > On Mon, Dec 05, 2022 at 01:12:03PM +0100, Greg Kroah-Hartman wrote:
> > > +/**
> > > + * container_of_const - cast a member of a structure out to the containing
> > > + *			structure and preserve the const-ness of the pointer
> > > + * @ptr:		the pointer to the member
> > > + * @type:		the type of the container struct this is embedded in.
> > > + * @member:		the name of the member within the struct.
> > > + */
> > > +#define container_of_const(ptr, type, member)				\
> > > +	_Generic(ptr,							\
> > > +		const typeof(*(ptr)) *: ((const type *)container_of(ptr, type, member)),\
> > > +		default: ((type *)container_of(ptr, type, member))	\
> > > +	)
> > > +
> > 
> > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > 
> > I tried doing this:
> > 
> > +++ b/include/linux/container_of.h
> > @@ -15,11 +15,17 @@
> >   *
> >   * WARNING: any const qualifier of @ptr is lost.
> >   */
> > -#define container_of(ptr, type, member) ({                             \
> > +#define _c_of(ptr, type, member) ({                                    \
> >         void *__mptr = (void *)(ptr);                                   \
> >         static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
> >                       __same_type(*(ptr), void),                        \
> >                       "pointer type mismatch in container_of()");       \
> >         ((type *)(__mptr - offsetof(type, member))); })
> > 
> > +#define container_of(ptr, type, m)                                     \
> > +       _Generic(ptr,                                                   \
> > +               const typeof(*(ptr)) *: (const type *)_c_of(ptr, type, m),\
> > +               default: ((type *)_c_of(ptr, type, m))                  \
> > +       )
> > +
> >  #endif /* _LINUX_CONTAINER_OF_H */
> > 
> > (whitespace damaged, yes the kernel-doc is now in the wrong place, etc)
> > 
> > It found a few problems; just building the mlx5 driver (I happened to be
> > doing some work on it in that tree).  We're definitely not ready to do
> > that yet, but I'll send a few patches to prepare for it.
> 
> Yeah, that's a good long-term goal to have here.  Once everything is
> moved over, switching all container_of_const() to just container_of()
> should be simple.

I found a problem in fs/dcache.c:

struct qstr {
        union {
                struct {
                        HASH_LEN_DECLARE;
                };
                u64 hash_len;
        };
        const unsigned char *name;
};

(note the const on "name")

static inline struct external_name *external_name(struct dentry *dentry)
{
        return container_of(dentry->d_name.name, struct external_name, name[0]);
}

dentry isn't const, but dentry->d_name.name is, so we match the const
case and the compiler emits a warning.  I don't think there's a way to
key off the constness of dentry instead of dentry->d_name.name, so
I've gone with the following for now.  Anybody prefer a different
approach?

diff --git a/fs/dcache.c b/fs/dcache.c
index 52e6d5fdab6b..b51a86f3cec6 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -288,7 +288,8 @@ struct external_name {
 
 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]);
 }
 
 static void __d_free(struct rcu_head *head)
@@ -329,7 +330,8 @@ void release_dentry_name_snapshot(struct name_snapshot *name)
 {
 	if (unlikely(name->name.name != name->inline_name)) {
 		struct external_name *p;
-		p = container_of(name->name.name, struct external_name, name[0]);
+		p = container_of_not_const(name->name.name,
+					   struct external_name, name[0]);
 		if (unlikely(atomic_dec_and_test(&p->u.count)))
 			kfree_rcu(p, u.head);
 	}
diff --git a/include/linux/container_of.h b/include/linux/container_of.h
index 23389af3af94..bf609a072754 100644
--- a/include/linux/container_of.h
+++ b/include/linux/container_of.h
@@ -25,4 +25,7 @@
 		const typeof(*(ptr)) *: (const type *)__mptr,		\
 		default: ((type *)__mptr)); })
 
+#define container_of_not_const(ptr, type, member) 			\
+	(type *)container_of(ptr, type, member)
+
 #endif	/* _LINUX_CONTAINER_OF_H */

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

[Greg Kroah-Hartman]

On Tue, Dec 06, 2022 at 05:18:22PM +0000, Matthew Wilcox wrote:
> On Tue, Dec 06, 2022 at 04:54:45PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Dec 06, 2022 at 03:13:29PM +0000, Matthew Wilcox wrote:
> > > On Mon, Dec 05, 2022 at 01:12:03PM +0100, Greg Kroah-Hartman wrote:
> > > > +/**
> > > > + * container_of_const - cast a member of a structure out to the containing
> > > > + *			structure and preserve the const-ness of the pointer
> > > > + * @ptr:		the pointer to the member
> > > > + * @type:		the type of the container struct this is embedded in.
> > > > + * @member:		the name of the member within the struct.
> > > > + */
> > > > +#define container_of_const(ptr, type, member)				\
> > > > +	_Generic(ptr,							\
> > > > +		const typeof(*(ptr)) *: ((const type *)container_of(ptr, type, member)),\
> > > > +		default: ((type *)container_of(ptr, type, member))	\
> > > > +	)
> > > > +
> > > 
> > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > > 
> > > I tried doing this:
> > > 
> > > +++ b/include/linux/container_of.h
> > > @@ -15,11 +15,17 @@
> > >   *
> > >   * WARNING: any const qualifier of @ptr is lost.
> > >   */
> > > -#define container_of(ptr, type, member) ({                             \
> > > +#define _c_of(ptr, type, member) ({                                    \
> > >         void *__mptr = (void *)(ptr);                                   \
> > >         static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
> > >                       __same_type(*(ptr), void),                        \
> > >                       "pointer type mismatch in container_of()");       \
> > >         ((type *)(__mptr - offsetof(type, member))); })
> > > 
> > > +#define container_of(ptr, type, m)                                     \
> > > +       _Generic(ptr,                                                   \
> > > +               const typeof(*(ptr)) *: (const type *)_c_of(ptr, type, m),\
> > > +               default: ((type *)_c_of(ptr, type, m))                  \
> > > +       )
> > > +
> > >  #endif /* _LINUX_CONTAINER_OF_H */
> > > 
> > > (whitespace damaged, yes the kernel-doc is now in the wrong place, etc)
> > > 
> > > It found a few problems; just building the mlx5 driver (I happened to be
> > > doing some work on it in that tree).  We're definitely not ready to do
> > > that yet, but I'll send a few patches to prepare for it.
> > 
> > Yeah, that's a good long-term goal to have here.  Once everything is
> > moved over, switching all container_of_const() to just container_of()
> > should be simple.
> 
> I found a problem in fs/dcache.c:
> 
> struct qstr {
>         union {
>                 struct {
>                         HASH_LEN_DECLARE;
>                 };
>                 u64 hash_len;
>         };
>         const unsigned char *name;
> };
> 
> (note the const on "name")
> 
> static inline struct external_name *external_name(struct dentry *dentry)
> {
>         return container_of(dentry->d_name.name, struct external_name, name[0]);
> }
> 
> dentry isn't const, but dentry->d_name.name is, so we match the const
> case and the compiler emits a warning.  I don't think there's a way to
> key off the constness of dentry instead of dentry->d_name.name, so
> I've gone with the following for now.  Anybody prefer a different
> approach?
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 52e6d5fdab6b..b51a86f3cec6 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -288,7 +288,8 @@ struct external_name {
>  
>  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 :)

>  static void __d_free(struct rcu_head *head)
> @@ -329,7 +330,8 @@ void release_dentry_name_snapshot(struct name_snapshot *name)
>  {
>  	if (unlikely(name->name.name != name->inline_name)) {
>  		struct external_name *p;
> -		p = container_of(name->name.name, struct external_name, name[0]);
> +		p = container_of_not_const(name->name.name,
> +					   struct external_name, name[0]);
>  		if (unlikely(atomic_dec_and_test(&p->u.count)))
>  			kfree_rcu(p, u.head);
>  	}
> diff --git a/include/linux/container_of.h b/include/linux/container_of.h
> index 23389af3af94..bf609a072754 100644
> --- a/include/linux/container_of.h
> +++ b/include/linux/container_of.h
> @@ -25,4 +25,7 @@
>  		const typeof(*(ptr)) *: (const type *)__mptr,		\
>  		default: ((type *)__mptr)); })
>  
> +#define container_of_not_const(ptr, type, member) 			\
> +	(type *)container_of(ptr, type, member)
> +

"not_const" feels odd, all you want to do is cast away the pointer
result here, right?

thanks,

greg k-h

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

[Matthew Wilcox]

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.

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

[Greg Kroah-Hartman]

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

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

[Rasmus Villemoes]

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.

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.

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.

Rasmus

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.