* [PATCH] lib/string_helpers: Use the given gfp flag when allocating memory
@ 2022-01-16 12:49 Christophe JAILLET
2022-01-16 12:54 ` Christophe JAILLET
2022-01-17 9:51 ` Michal Hocko
0 siblings, 2 replies; 5+ messages in thread
From: Christophe JAILLET @ 2022-01-16 12:49 UTC (permalink / raw)
To: Andy Shevchenko, Stephen Rothwell, Andrew Morton, Mel Gorman,
Michal Hocko, Vlastimil Babka
Cc: linux-kernel, kernel-janitors, Christophe JAILLET
kstrdup_quotable_cmdline() is given a gfp flag that is passed and used for
memory allocation in kstrdup_quotable() just a few lines below.
It looks reasonable to use this gfp value for the buffer allocated and
freed in kstrdup_quotable_cmdline() as well.
Fixes: 0ee931c4e31a ("mm: treewide: remove GFP_TEMPORARY allocation flag")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
According to what I've found in 5.16, all callers use GFP_KERNEL, so this
patch should be a no-op.
But who knows how it will be used in the future. Better safe than sorry.
---
lib/string_helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 90f9f1b7afec..7aceeb40dfd7 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -624,7 +624,7 @@ char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp)
char *buffer, *quoted;
int i, res;
- buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
+ buffer = kmalloc(PAGE_SIZE, gfp);
if (!buffer)
return NULL;
--
2.32.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] lib/string_helpers: Use the given gfp flag when allocating memory
2022-01-16 12:49 [PATCH] lib/string_helpers: Use the given gfp flag when allocating memory Christophe JAILLET
@ 2022-01-16 12:54 ` Christophe JAILLET
2022-01-17 9:51 ` Michal Hocko
1 sibling, 0 replies; 5+ messages in thread
From: Christophe JAILLET @ 2022-01-16 12:54 UTC (permalink / raw)
To: Andy Shevchenko, Stephen Rothwell, Andrew Morton, Mel Gorman,
Michal Hocko, Vlastimil Babka
Cc: linux-kernel, kernel-janitors
Le 16/01/2022 à 13:49, Christophe JAILLET a écrit :
> kstrdup_quotable_cmdline() is given a gfp flag that is passed and used for
> memory allocation in kstrdup_quotable() just a few lines below.
>
> It looks reasonable to use this gfp value for the buffer allocated and
> freed in kstrdup_quotable_cmdline() as well.
>
> Fixes: 0ee931c4e31a ("mm: treewide: remove GFP_TEMPORARY allocation flag")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> According to what I've found in 5.16, all callers use GFP_KERNEL, so this
> patch should be a no-op.
> But who knows how it will be used in the future. Better safe than sorry.
> ---
> lib/string_helpers.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 90f9f1b7afec..7aceeb40dfd7 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -624,7 +624,7 @@ char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp)
> char *buffer, *quoted;
> int i, res;
>
> - buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
> + buffer = kmalloc(PAGE_SIZE, gfp);
> if (!buffer)
> return NULL;
>
>
I have sent a V2. I missed the same issue in kstrdup_quotable_file().
CJ
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lib/string_helpers: Use the given gfp flag when allocating memory
2022-01-16 12:49 [PATCH] lib/string_helpers: Use the given gfp flag when allocating memory Christophe JAILLET
2022-01-16 12:54 ` Christophe JAILLET
@ 2022-01-17 9:51 ` Michal Hocko
2022-01-17 15:42 ` Michal Hocko
1 sibling, 1 reply; 5+ messages in thread
From: Michal Hocko @ 2022-01-17 9:51 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Andy Shevchenko, Stephen Rothwell, Andrew Morton, Mel Gorman,
Vlastimil Babka, linux-kernel, kernel-janitors
On Sun 16-01-22 13:49:22, Christophe JAILLET wrote:
> kstrdup_quotable_cmdline() is given a gfp flag that is passed and used for
> memory allocation in kstrdup_quotable() just a few lines below.
>
> It looks reasonable to use this gfp value for the buffer allocated and
> freed in kstrdup_quotable_cmdline() as well.
>
> Fixes: 0ee931c4e31a ("mm: treewide: remove GFP_TEMPORARY allocation flag")
I do not think this commit is changing much here. It just replaces
GFP_TEMPORARY with GFP_KERNEL so the code has ignored the gfp mask even
before that change.
All existing callers of kstrdup_quotable_cmdline use GFP_KERNEL so would
it make more sense to simply drop the gfp argument altogether and use
GFP_KERNEL internally?
Normally it is better to have a full control of the allocation mask but
if we have any non-GFP_KERNEL caller then I would rather have the
argument added and the function checked whether all internal paths are
gfp mask aware.
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> According to what I've found in 5.16, all callers use GFP_KERNEL, so this
> patch should be a no-op.
> But who knows how it will be used in the future. Better safe than sorry.
> ---
> lib/string_helpers.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 90f9f1b7afec..7aceeb40dfd7 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -624,7 +624,7 @@ char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp)
> char *buffer, *quoted;
> int i, res;
>
> - buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
> + buffer = kmalloc(PAGE_SIZE, gfp);
> if (!buffer)
> return NULL;
>
> --
> 2.32.0
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lib/string_helpers: Use the given gfp flag when allocating memory
2022-01-17 9:51 ` Michal Hocko
@ 2022-01-17 15:42 ` Michal Hocko
2022-01-17 19:29 ` Christophe JAILLET
0 siblings, 1 reply; 5+ messages in thread
From: Michal Hocko @ 2022-01-17 15:42 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Andy Shevchenko, Stephen Rothwell, Andrew Morton, Mel Gorman,
Vlastimil Babka, linux-kernel, kernel-janitors
On Mon 17-01-22 10:51:59, Michal Hocko wrote:
> On Sun 16-01-22 13:49:22, Christophe JAILLET wrote:
> > kstrdup_quotable_cmdline() is given a gfp flag that is passed and used for
> > memory allocation in kstrdup_quotable() just a few lines below.
> >
> > It looks reasonable to use this gfp value for the buffer allocated and
> > freed in kstrdup_quotable_cmdline() as well.
> >
> > Fixes: 0ee931c4e31a ("mm: treewide: remove GFP_TEMPORARY allocation flag")
>
> I do not think this commit is changing much here. It just replaces
> GFP_TEMPORARY with GFP_KERNEL so the code has ignored the gfp mask even
> before that change.
>
> All existing callers of kstrdup_quotable_cmdline use GFP_KERNEL so would
> it make more sense to simply drop the gfp argument altogether and use
> GFP_KERNEL internally?
>
> Normally it is better to have a full control of the allocation mask but
> if we have any non-GFP_KERNEL caller then I would rather have the
> argument added and the function checked whether all internal paths are
> gfp mask aware.
In other words something like this:
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 2c46cd968ac4..44fde4b537f1 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -376,7 +376,7 @@ static void recover_worker(struct kthread_work *work)
task = get_pid_task(submit->pid, PIDTYPE_PID);
if (task) {
comm = kstrdup(task->comm, GFP_KERNEL);
- cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
+ cmd = kstrdup_quotable_cmdline(task);
put_task_struct(task);
}
@@ -467,7 +467,7 @@ static void fault_worker(struct kthread_work *work)
task = get_pid_task(submit->pid, PIDTYPE_PID);
if (task) {
comm = kstrdup(task->comm, GFP_KERNEL);
- cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
+ cmd = kstrdup_quotable_cmdline(task);
put_task_struct(task);
}
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 4ba39e1403b2..7a67eee8bd0f 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -97,8 +97,8 @@ static inline void string_lower(char *dst, const char *src)
}
char *kstrdup_quotable(const char *src, gfp_t gfp);
-char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp);
-char *kstrdup_quotable_file(struct file *file, gfp_t gfp);
+char *kstrdup_quotable_cmdline(struct task_struct *task);
+char *kstrdup_quotable_file(struct file *file);
void kfree_strarray(char **array, size_t n);
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index d5d008f5b1d9..267e142c7e13 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -618,12 +618,13 @@ EXPORT_SYMBOL_GPL(kstrdup_quotable);
* command line, with inter-argument NULLs replaced with spaces,
* and other special characters escaped.
*/
-char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp)
+char *kstrdup_quotable_cmdline(struct task_struct *task)
{
+ gfp_t gfp = GFP_KERNEL;
char *buffer, *quoted;
int i, res;
- buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
+ buffer = kmalloc(PAGE_SIZE, gfp);
if (!buffer)
return NULL;
@@ -651,15 +652,16 @@ EXPORT_SYMBOL_GPL(kstrdup_quotable_cmdline);
* with special characters escaped, able to be safely logged. If
* there is an error, the leading character will be "<".
*/
-char *kstrdup_quotable_file(struct file *file, gfp_t gfp)
+char *kstrdup_quotable_file(struct file *file)
{
+ gfp_t gfp = GFP_KERNEL;
char *temp, *pathname;
if (!file)
return kstrdup("<unknown>", gfp);
/* We add 11 spaces for ' (deleted)' to be appended */
- temp = kmalloc(PATH_MAX + 11, GFP_KERNEL);
+ temp = kmalloc(PATH_MAX + 11, gfp);
if (!temp)
return kstrdup("<no_memory>", gfp);
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index b12f7d986b1e..79322ba89913 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -23,8 +23,8 @@ static void report_load(const char *origin, struct file *file, char *operation)
{
char *cmdline, *pathname;
- pathname = kstrdup_quotable_file(file, GFP_KERNEL);
- cmdline = kstrdup_quotable_cmdline(current, GFP_KERNEL);
+ pathname = kstrdup_quotable_file(file);
+ cmdline = kstrdup_quotable_cmdline(current);
pr_notice("%s %s obj=%s%s%s pid=%d cmdline=%s%s%s\n",
origin, operation,
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 06e226166aab..c87a41304b6c 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -54,8 +54,8 @@ static void __report_access(struct callback_head *work)
container_of(work, struct access_report_info, work);
char *target_cmd, *agent_cmd;
- target_cmd = kstrdup_quotable_cmdline(info->target, GFP_KERNEL);
- agent_cmd = kstrdup_quotable_cmdline(info->agent, GFP_KERNEL);
+ target_cmd = kstrdup_quotable_cmdline(info->target);
+ agent_cmd = kstrdup_quotable_cmdline(info->agent);
pr_notice_ratelimited(
"ptrace %s of \"%s\"[%d] was attempted by \"%s\"[%d]\n",
--
Michal Hocko
SUSE Labs
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] lib/string_helpers: Use the given gfp flag when allocating memory
2022-01-17 15:42 ` Michal Hocko
@ 2022-01-17 19:29 ` Christophe JAILLET
0 siblings, 0 replies; 5+ messages in thread
From: Christophe JAILLET @ 2022-01-17 19:29 UTC (permalink / raw)
To: Michal Hocko
Cc: Andy Shevchenko, Stephen Rothwell, Andrew Morton, Mel Gorman,
Vlastimil Babka, linux-kernel, kernel-janitors
Le 17/01/2022 à 16:42, Michal Hocko a écrit :
> On Mon 17-01-22 10:51:59, Michal Hocko wrote:
>> On Sun 16-01-22 13:49:22, Christophe JAILLET wrote:
>>> kstrdup_quotable_cmdline() is given a gfp flag that is passed and used for
>>> memory allocation in kstrdup_quotable() just a few lines below.
>>>
>>> It looks reasonable to use this gfp value for the buffer allocated and
>>> freed in kstrdup_quotable_cmdline() as well.
>>>
>>> Fixes: 0ee931c4e31a ("mm: treewide: remove GFP_TEMPORARY allocation flag")
>>
>> I do not think this commit is changing much here. It just replaces
>> GFP_TEMPORARY with GFP_KERNEL so the code has ignored the gfp mask even
>> before that change.
>>
>> All existing callers of kstrdup_quotable_cmdline use GFP_KERNEL so would
>> it make more sense to simply drop the gfp argument altogether and use
>> GFP_KERNEL internally?
>>
>> Normally it is better to have a full control of the allocation mask but
>> if we have any non-GFP_KERNEL caller then I would rather have the
>> argument added and the function checked whether all internal paths are
>> gfp mask aware.
>
> In other words something like this:
For me, this is just fine.
This looks more consistant this way and (slighly) simplify the few callers.
CJ
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 2c46cd968ac4..44fde4b537f1 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -376,7 +376,7 @@ static void recover_worker(struct kthread_work *work)
> task = get_pid_task(submit->pid, PIDTYPE_PID);
> if (task) {
> comm = kstrdup(task->comm, GFP_KERNEL);
> - cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
> + cmd = kstrdup_quotable_cmdline(task);
> put_task_struct(task);
> }
>
> @@ -467,7 +467,7 @@ static void fault_worker(struct kthread_work *work)
> task = get_pid_task(submit->pid, PIDTYPE_PID);
> if (task) {
> comm = kstrdup(task->comm, GFP_KERNEL);
> - cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
> + cmd = kstrdup_quotable_cmdline(task);
> put_task_struct(task);
> }
>
> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
> index 4ba39e1403b2..7a67eee8bd0f 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -97,8 +97,8 @@ static inline void string_lower(char *dst, const char *src)
> }
>
> char *kstrdup_quotable(const char *src, gfp_t gfp);
> -char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp);
> -char *kstrdup_quotable_file(struct file *file, gfp_t gfp);
> +char *kstrdup_quotable_cmdline(struct task_struct *task);
> +char *kstrdup_quotable_file(struct file *file);
>
> void kfree_strarray(char **array, size_t n);
>
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index d5d008f5b1d9..267e142c7e13 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -618,12 +618,13 @@ EXPORT_SYMBOL_GPL(kstrdup_quotable);
> * command line, with inter-argument NULLs replaced with spaces,
> * and other special characters escaped.
> */
> -char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp)
> +char *kstrdup_quotable_cmdline(struct task_struct *task)
> {
> + gfp_t gfp = GFP_KERNEL;
> char *buffer, *quoted;
> int i, res;
>
> - buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
> + buffer = kmalloc(PAGE_SIZE, gfp);
> if (!buffer)
> return NULL;
>
> @@ -651,15 +652,16 @@ EXPORT_SYMBOL_GPL(kstrdup_quotable_cmdline);
> * with special characters escaped, able to be safely logged. If
> * there is an error, the leading character will be "<".
> */
> -char *kstrdup_quotable_file(struct file *file, gfp_t gfp)
> +char *kstrdup_quotable_file(struct file *file)
> {
> + gfp_t gfp = GFP_KERNEL;
> char *temp, *pathname;
>
> if (!file)
> return kstrdup("<unknown>", gfp);
>
> /* We add 11 spaces for ' (deleted)' to be appended */
> - temp = kmalloc(PATH_MAX + 11, GFP_KERNEL);
> + temp = kmalloc(PATH_MAX + 11, gfp);
> if (!temp)
> return kstrdup("<no_memory>", gfp);
>
> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> index b12f7d986b1e..79322ba89913 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -23,8 +23,8 @@ static void report_load(const char *origin, struct file *file, char *operation)
> {
> char *cmdline, *pathname;
>
> - pathname = kstrdup_quotable_file(file, GFP_KERNEL);
> - cmdline = kstrdup_quotable_cmdline(current, GFP_KERNEL);
> + pathname = kstrdup_quotable_file(file);
> + cmdline = kstrdup_quotable_cmdline(current);
>
> pr_notice("%s %s obj=%s%s%s pid=%d cmdline=%s%s%s\n",
> origin, operation,
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index 06e226166aab..c87a41304b6c 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -54,8 +54,8 @@ static void __report_access(struct callback_head *work)
> container_of(work, struct access_report_info, work);
> char *target_cmd, *agent_cmd;
>
> - target_cmd = kstrdup_quotable_cmdline(info->target, GFP_KERNEL);
> - agent_cmd = kstrdup_quotable_cmdline(info->agent, GFP_KERNEL);
> + target_cmd = kstrdup_quotable_cmdline(info->target);
> + agent_cmd = kstrdup_quotable_cmdline(info->agent);
>
> pr_notice_ratelimited(
> "ptrace %s of \"%s\"[%d] was attempted by \"%s\"[%d]\n",
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-17 19:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-16 12:49 [PATCH] lib/string_helpers: Use the given gfp flag when allocating memory Christophe JAILLET
2022-01-16 12:54 ` Christophe JAILLET
2022-01-17 9:51 ` Michal Hocko
2022-01-17 15:42 ` Michal Hocko
2022-01-17 19:29 ` Christophe JAILLET
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).