| Message ID | 20240326145140.3257163-4-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kbuild: enable some -Wextra warnings by default | expand |
Hi Arnd, On Tue, 26 Mar 2024 15:51:30 +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > dmi_class uses kfree() as the .release function, but that now causes > a warning with clang-16 as it violates control flow integrity (KCFI) > rules: > > drivers/firmware/dmi-id.c:174:17: error: cast from 'void (*)(const void *)' to 'void (*)(struct device *)' converts to incompatible function type [-Werror,-Wcast-function-type-strict] > 174 | .dev_release = (void(*)(struct device *)) kfree, > > Add an explicit function to call kfree() instead. > > Fixes: 4f5c791a850e ("DMI-based module autoloading") Not sure if this fixes tag is really warranted. As I understand it, your change only removes a warning but there was no actual bug, right? > Link: https://lore.kernel.org/lkml/20240213100238.456912-1-arnd@kernel.org/ > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > I sent this before but got no comments for it I indeed overlooked your initial submission, my bad. > --- > drivers/firmware/dmi-id.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c > index 5f3a3e913d28..d19c78a78ae3 100644 > --- a/drivers/firmware/dmi-id.c > +++ b/drivers/firmware/dmi-id.c > @@ -169,9 +169,14 @@ static int dmi_dev_uevent(const struct device *dev, struct kobj_uevent_env *env) > return 0; > } > > +static void dmi_dev_release(struct device *dev) > +{ > + kfree(dev); > +} > + > static struct class dmi_class = { > .name = "dmi", > - .dev_release = (void(*)(struct device *)) kfree, > + .dev_release = dmi_dev_release, > .dev_uevent = dmi_dev_uevent, > }; > Looks good to me, thanks for doing that. Signed-off-by: Jean Delvare <jdelvare@suse.de> Will you get this upstream, or do you expect me to take it in my dmi/for-next branch?
On Fri, Mar 29, 2024 at 01:49:17PM +0100, Jean Delvare wrote: > Hi Arnd, > > On Tue, 26 Mar 2024 15:51:30 +0100, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > dmi_class uses kfree() as the .release function, but that now causes > > a warning with clang-16 as it violates control flow integrity (KCFI) > > rules: > > > > drivers/firmware/dmi-id.c:174:17: error: cast from 'void (*)(const void *)' to 'void (*)(struct device *)' converts to incompatible function type [-Werror,-Wcast-function-type-strict] > > 174 | .dev_release = (void(*)(struct device *)) kfree, > > > > Add an explicit function to call kfree() instead. > > > > Fixes: 4f5c791a850e ("DMI-based module autoloading") > > Not sure if this fixes tag is really warranted. As I understand it, > your change only removes a warning but there was no actual bug, right? Sort of, the warning is really pointing out that calling this callback will result in a crash at runtime when the kernel is built with kCFI enabled, which I would consider a bug. Cheers, Nathan
On Fri, Mar 29, 2024, at 13:49, Jean Delvare wrote: > On Tue, 26 Mar 2024 15:51:30 +0100, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> dmi_class uses kfree() as the .release function, but that now causes >> a warning with clang-16 as it violates control flow integrity (KCFI) >> rules: >> >> drivers/firmware/dmi-id.c:174:17: error: cast from 'void (*)(const void *)' to 'void (*)(struct device *)' converts to incompatible function type [-Werror,-Wcast-function-type-strict] >> 174 | .dev_release = (void(*)(struct device *)) kfree, >> >> Add an explicit function to call kfree() instead. >> >> Fixes: 4f5c791a850e ("DMI-based module autoloading") > > Not sure if this fixes tag is really warranted. As I understand it, > your change only removes a warning but there was no actual bug, right? As Nathan already commented, it's a real bug. I also add 'Fixes' tags for false-positives just to document what introduced a warning. The Fixes tag doesn't automatically mean something gets backported, though the stable maintainers often end up backporting warning fixes as well, and it helps identify which kernels need it. > Looks good to me, thanks for doing that. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > > Will you get this upstream, or do you expect me to take it in my > dmi/for-next branch? It would help me if you can apply it to your tree directly. Arnd
Hi Arnd, On Thu, 04 Apr 2024 16:07:55 +0200, Arnd Bergmann wrote: > On Fri, Mar 29, 2024, at 13:49, Jean Delvare wrote: > > Will you get this upstream, or do you expect me to take it in my > > dmi/for-next branch? > > It would help me if you can apply it to your tree directly. OK, it's in my dmi-for-next branch now: https://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git/log/?h=dmi-for-next Thanks,
On Mon, Apr 8, 2024, at 09:59, Jean Delvare wrote: > On Thu, 04 Apr 2024 16:07:55 +0200, Arnd Bergmann wrote: >> On Fri, Mar 29, 2024, at 13:49, Jean Delvare wrote: >> > Will you get this upstream, or do you expect me to take it in my >> > dmi/for-next branch? >> >> It would help me if you can apply it to your tree directly. > > OK, it's in my dmi-for-next branch now: > > https://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git/log/?h=dmi-for-next Hi Jean, I see it's in your tree, but I don't see your tree in linux-next. As all the other fixes from my series got merged, I would like to merge the patches that turn the warnings on, but that still causes a build-time regression. Is there a reason for the dmi-next tree not being part of linux-next, or should we ask Stephen (added to Cc) to add it? Arnd
On Fri, 12 Apr 2024 11:42:03 +0200, Arnd Bergmann wrote: > On Mon, Apr 8, 2024, at 09:59, Jean Delvare wrote: > > On Thu, 04 Apr 2024 16:07:55 +0200, Arnd Bergmann wrote: > >> On Fri, Mar 29, 2024, at 13:49, Jean Delvare wrote: > >> > Will you get this upstream, or do you expect me to take it in my > >> > dmi/for-next branch? > >> > >> It would help me if you can apply it to your tree directly. > > > > OK, it's in my dmi-for-next branch now: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git/log/?h=dmi-for-next > > I see it's in your tree, but I don't see your tree in linux-next. > > As all the other fixes from my series got merged, I would like to > merge the patches that turn the warnings on, but that still > causes a build-time regression. > > Is there a reason for the dmi-next tree not being part of > linux-next, or should we ask Stephen (added to Cc) to add it? Hmm, Stephen cleaned up the list of trees he pulls from 2 months ago. Back then, I objected that I may use my tree again in the future, and I thought he had added it back to the list, but maybe I misunderstood. Stephen, can you check if you still pull from tree above, and if not, add it back to the list? Thanks in advance.
Hi Jean, On Sat, 13 Apr 2024 22:38:57 +0200 Jean Delvare <jdelvare@suse.de> wrote: > > Hmm, Stephen cleaned up the list of trees he pulls from 2 months ago. > Back then, I objected that I may use my tree again in the future, and I > thought he had added it back to the list, but maybe I misunderstood. > > Stephen, can you check if you still pull from tree above, and if not, > add it back to the list? Thanks in advance. I also misunderstood your position at the time. I have now restored the dmi tree to linux-next.
diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c index 5f3a3e913d28..d19c78a78ae3 100644 --- a/drivers/firmware/dmi-id.c +++ b/drivers/firmware/dmi-id.c @@ -169,9 +169,14 @@ static int dmi_dev_uevent(const struct device *dev, struct kobj_uevent_env *env) return 0; } +static void dmi_dev_release(struct device *dev) +{ + kfree(dev); +} + static struct class dmi_class = { .name = "dmi", - .dev_release = (void(*)(struct device *)) kfree, + .dev_release = dmi_dev_release, .dev_uevent = dmi_dev_uevent, };