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 <[email protected]>
Suggested-by: Sakari Ailus <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
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
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 <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
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
On Mon, Dec 5, 2022 at 1:12 PM Greg Kroah-Hartman
<[email protected]> 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 <[email protected]>
> Suggested-by: Sakari Ailus <[email protected]>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
> ---
> 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
>
On Mon, Dec 5, 2022 at 1:12 PM Greg Kroah-Hartman
<[email protected]> 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 <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
> ---
> 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
>
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 <[email protected]>
> Suggested-by: Sakari Ailus <[email protected]>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> 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 <[email protected]>
>
> 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
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) <[email protected]>
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.
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) <[email protected]>
>
> 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
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) <[email protected]>
> >
> > 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 */
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) <[email protected]>
> > >
> > > 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
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.
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
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
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.