[PATCH v7] devcoredump: change gfp_t parameter of kzalloc to GFP_KERNEL

[PATCH v7] devcoredump: change gfp_t parameter of kzalloc to GFP_KERNEL

[Duoming Zhou]

The dev_coredumpv() and dev_coredumpm() could not be used in atomic
context, because they call kvasprintf_const() and kstrdup() with
GFP_KERNEL parameter. The process is shown below:

dev_coredumpv(.., gfp_t gfp)
  dev_coredumpm(.., gfp_t gfp)
    kzalloc(.., gfp);
    dev_set_name
      kobject_set_name_vargs
        kvasprintf_const(GFP_KERNEL, ...); //may sleep
          kstrdup(s, GFP_KERNEL); //may sleep

This patch changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
GFP_KERNEL in order to show they could not be used in atomic context.

What's more, this patch does not remove the gfp_t parameter in
dev_coredumpv() and dev_coredumpm() in order that it will not influence
other new users that are added in other trees.

Fixes: 833c95456a70 ("device coredump: add new device coredump class")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
Changes in v7:
  - change gfp_t parameter of kzalloc in dev_coredumpm() to GFP_KERNEL.

 drivers/base/devcoredump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
index f4d794d6bb8..cf60aacf8a8 100644
--- a/drivers/base/devcoredump.c
+++ b/drivers/base/devcoredump.c
@@ -268,7 +268,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
 	if (!try_module_get(owner))
 		goto free;
 
-	devcd = kzalloc(sizeof(*devcd), gfp);
+	devcd = kzalloc(sizeof(*devcd), GFP_KERNEL);
 	if (!devcd)
 		goto put_module;
 
-- 
2.17.1

Re: [PATCH v7] devcoredump: change gfp_t parameter of kzalloc to GFP_KERNEL

[Greg KH]

On Tue, Jun 28, 2022 at 11:44:58AM +0800, Duoming Zhou wrote:
> The dev_coredumpv() and dev_coredumpm() could not be used in atomic
> context, because they call kvasprintf_const() and kstrdup() with
> GFP_KERNEL parameter. The process is shown below:
> 
> dev_coredumpv(.., gfp_t gfp)
>   dev_coredumpm(.., gfp_t gfp)
>     kzalloc(.., gfp);
>     dev_set_name
>       kobject_set_name_vargs
>         kvasprintf_const(GFP_KERNEL, ...); //may sleep
>           kstrdup(s, GFP_KERNEL); //may sleep
> 
> This patch changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
> GFP_KERNEL in order to show they could not be used in atomic context.
> 
> What's more, this patch does not remove the gfp_t parameter in
> dev_coredumpv() and dev_coredumpm() in order that it will not influence
> other new users that are added in other trees.
> 
> Fixes: 833c95456a70 ("device coredump: add new device coredump class")
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
> Changes in v7:
>   - change gfp_t parameter of kzalloc in dev_coredumpm() to GFP_KERNEL.
> 
>  drivers/base/devcoredump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
> index f4d794d6bb8..cf60aacf8a8 100644
> --- a/drivers/base/devcoredump.c
> +++ b/drivers/base/devcoredump.c
> @@ -268,7 +268,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
>  	if (!try_module_get(owner))
>  		goto free;
>  
> -	devcd = kzalloc(sizeof(*devcd), gfp);
> +	devcd = kzalloc(sizeof(*devcd), GFP_KERNEL);

No, you can't just ignore the flag entirely, that doesn't help anyone
out who tries to set it and is totally confused as to why the field is
ignored.

You need to evolve the function over time to not need the parameter at
all, this just papers over the entire issue, which makes the api lie to
the caller, not something you ever want to do.

thanks,

greg k-h

Re: [PATCH v7] devcoredump: change gfp_t parameter of kzalloc to GFP_KERNEL

[duoming]

hello,

On Tue, 28 Jun 2022 08:09:06 +0200 greg KH wrote:

> On Tue, Jun 28, 2022 at 11:44:58AM +0800, Duoming Zhou wrote:
> > The dev_coredumpv() and dev_coredumpm() could not be used in atomic
> > context, because they call kvasprintf_const() and kstrdup() with
> > GFP_KERNEL parameter. The process is shown below:
> > 
> > dev_coredumpv(.., gfp_t gfp)
> >   dev_coredumpm(.., gfp_t gfp)
> >     kzalloc(.., gfp);
> >     dev_set_name
> >       kobject_set_name_vargs
> >         kvasprintf_const(GFP_KERNEL, ...); //may sleep
> >           kstrdup(s, GFP_KERNEL); //may sleep
> > 
> > This patch changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
> > GFP_KERNEL in order to show they could not be used in atomic context.
> > 
> > What's more, this patch does not remove the gfp_t parameter in
> > dev_coredumpv() and dev_coredumpm() in order that it will not influence
> > other new users that are added in other trees.
> > 
> > Fixes: 833c95456a70 ("device coredump: add new device coredump class")
> > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> > ---
> > Changes in v7:
> >   - change gfp_t parameter of kzalloc in dev_coredumpm() to GFP_KERNEL.
> > 
> >  drivers/base/devcoredump.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
> > index f4d794d6bb8..cf60aacf8a8 100644
> > --- a/drivers/base/devcoredump.c
> > +++ b/drivers/base/devcoredump.c
> > @@ -268,7 +268,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
> >  	if (!try_module_get(owner))
> >  		goto free;
> >  
> > -	devcd = kzalloc(sizeof(*devcd), gfp);
> > +	devcd = kzalloc(sizeof(*devcd), GFP_KERNEL);
> 
> No, you can't just ignore the flag entirely, that doesn't help anyone
> out who tries to set it and is totally confused as to why the field is
> ignored.
> 
> You need to evolve the function over time to not need the parameter at
> all, this just papers over the entire issue, which makes the api lie to
> the caller, not something you ever want to do.

Thank you for your time and reply.

But if there are new devices come into kernel, it may use devcoredump api.
What is the proper time to remove the gfp_t parameter of dev_coredumpv()
and dev_coredumpm()?

Best regards,
Duoming Zhou

Re: [PATCH v7] devcoredump: change gfp_t parameter of kzalloc to GFP_KERNEL

[Greg KH]

On Tue, Jun 28, 2022 at 07:06:39PM +0800, duoming@zju.edu.cn wrote:
> hello,
> 
> On Tue, 28 Jun 2022 08:09:06 +0200 greg KH wrote:
> 
> > On Tue, Jun 28, 2022 at 11:44:58AM +0800, Duoming Zhou wrote:
> > > The dev_coredumpv() and dev_coredumpm() could not be used in atomic
> > > context, because they call kvasprintf_const() and kstrdup() with
> > > GFP_KERNEL parameter. The process is shown below:
> > > 
> > > dev_coredumpv(.., gfp_t gfp)
> > >   dev_coredumpm(.., gfp_t gfp)
> > >     kzalloc(.., gfp);
> > >     dev_set_name
> > >       kobject_set_name_vargs
> > >         kvasprintf_const(GFP_KERNEL, ...); //may sleep
> > >           kstrdup(s, GFP_KERNEL); //may sleep
> > > 
> > > This patch changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
> > > GFP_KERNEL in order to show they could not be used in atomic context.
> > > 
> > > What's more, this patch does not remove the gfp_t parameter in
> > > dev_coredumpv() and dev_coredumpm() in order that it will not influence
> > > other new users that are added in other trees.
> > > 
> > > Fixes: 833c95456a70 ("device coredump: add new device coredump class")
> > > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> > > ---
> > > Changes in v7:
> > >   - change gfp_t parameter of kzalloc in dev_coredumpm() to GFP_KERNEL.
> > > 
> > >  drivers/base/devcoredump.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
> > > index f4d794d6bb8..cf60aacf8a8 100644
> > > --- a/drivers/base/devcoredump.c
> > > +++ b/drivers/base/devcoredump.c
> > > @@ -268,7 +268,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
> > >  	if (!try_module_get(owner))
> > >  		goto free;
> > >  
> > > -	devcd = kzalloc(sizeof(*devcd), gfp);
> > > +	devcd = kzalloc(sizeof(*devcd), GFP_KERNEL);
> > 
> > No, you can't just ignore the flag entirely, that doesn't help anyone
> > out who tries to set it and is totally confused as to why the field is
> > ignored.
> > 
> > You need to evolve the function over time to not need the parameter at
> > all, this just papers over the entire issue, which makes the api lie to
> > the caller, not something you ever want to do.
> 
> Thank you for your time and reply.
> 
> But if there are new devices come into kernel, it may use devcoredump api.
> What is the proper time to remove the gfp_t parameter of dev_coredumpv()
> and dev_coredumpm()?

Normally you prepare some patches that does the conversion as a patch
series and I queue them up in my tree, and get them merged in -rc1, then
any stragglers are then fixed up in -rc2 along with the final rename of
the old way and then all is good.  See lots of examples of changing apis
over time on the mailing lists for how to do this.

thanks,

greg k-h

Re: [PATCH v7] devcoredump: change gfp_t parameter of kzalloc to GFP_KERNEL

[duoming]

Hello,

On Tue, 28 Jun 2022 14:18:29 +0200 greg KH wrote:

> > > > The dev_coredumpv() and dev_coredumpm() could not be used in atomic
> > > > context, because they call kvasprintf_const() and kstrdup() with
> > > > GFP_KERNEL parameter. The process is shown below:
> > > > 
> > > > dev_coredumpv(.., gfp_t gfp)
> > > >   dev_coredumpm(.., gfp_t gfp)
> > > >     kzalloc(.., gfp);
> > > >     dev_set_name
> > > >       kobject_set_name_vargs
> > > >         kvasprintf_const(GFP_KERNEL, ...); //may sleep
> > > >           kstrdup(s, GFP_KERNEL); //may sleep
> > > > 
> > > > This patch changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
> > > > GFP_KERNEL in order to show they could not be used in atomic context.
> > > > 
> > > > What's more, this patch does not remove the gfp_t parameter in
> > > > dev_coredumpv() and dev_coredumpm() in order that it will not influence
> > > > other new users that are added in other trees.
> > > > 
> > > > Fixes: 833c95456a70 ("device coredump: add new device coredump class")
> > > > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> > > > ---
> > > > Changes in v7:
> > > >   - change gfp_t parameter of kzalloc in dev_coredumpm() to GFP_KERNEL.
> > > > 
> > > >  drivers/base/devcoredump.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
> > > > index f4d794d6bb8..cf60aacf8a8 100644
> > > > --- a/drivers/base/devcoredump.c
> > > > +++ b/drivers/base/devcoredump.c
> > > > @@ -268,7 +268,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
> > > >  	if (!try_module_get(owner))
> > > >  		goto free;
> > > >  
> > > > -	devcd = kzalloc(sizeof(*devcd), gfp);
> > > > +	devcd = kzalloc(sizeof(*devcd), GFP_KERNEL);
> > > 
> > > No, you can't just ignore the flag entirely, that doesn't help anyone
> > > out who tries to set it and is totally confused as to why the field is
> > > ignored.
> > > 
> > > You need to evolve the function over time to not need the parameter at
> > > all, this just papers over the entire issue, which makes the api lie to
> > > the caller, not something you ever want to do.
> > 
> > Thank you for your time and reply.
> > 
> > But if there are new devices come into kernel, it may use devcoredump api.
> > What is the proper time to remove the gfp_t parameter of dev_coredumpv()
> > and dev_coredumpm()?
> 
> Normally you prepare some patches that does the conversion as a patch
> series and I queue them up in my tree, and get them merged in -rc1, then
> any stragglers are then fixed up in -rc2 along with the final rename of
> the old way and then all is good.  See lots of examples of changing apis
> over time on the mailing lists for how to do this.

Thank you for your time and reply, I understand.

I will resend the patch that removes the gfp_t parameter of dev_coredumpv() and dev_coredumpm()
until commit oc3d8785f6c04a ("drm/amdgpu: adding device coredump support") has been merged into
mainline. Maybe after v5.19-rc5 or v5.19-rc6.

Best regards,
Duoming Zhou