[PATCH] workqueue.c: Change workqueue to accept variable length name

[PATCH] workqueue.c: Change workqueue to accept variable length name

[Audra Mitchell]

Currently we limit the size of the workqueue name to 24 characters due to
commit ecf6881ff349 ("workqueue: make workqueue->name[] fixed len")
As device names increase in size a static size for the workqueue name no
longer satisfies the size requirement, leading to truncated workqueue
names as we append the device name to the workqueue name. Truncation of
the workqueue names can cause issues when debugging as each is unique to
the associated device. Bring back the flexibility of a variable length
workqueue name to prevent truncation.

Signed-off-by: Audra Mitchell <audra@redhat.com>
---
 kernel/workqueue.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 2989b57e154a..6e9e332d1cf4 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -107,8 +107,6 @@ enum {
 	 */
 	RESCUER_NICE_LEVEL	= MIN_NICE,
 	HIGHPRI_NICE_LEVEL	= MIN_NICE,
-
-	WQ_NAME_LEN		= 24,
 };
 
 /*
@@ -311,7 +309,7 @@ struct workqueue_struct {
 	struct lock_class_key	key;
 	struct lockdep_map	lockdep_map;
 #endif
-	char			name[WQ_NAME_LEN]; /* I: workqueue name */
+	char			*name; /* I: workqueue name */
 
 	/*
 	 * Destruction of workqueue_struct is RCU protected to allow walking
@@ -3929,6 +3927,7 @@ static void rcu_free_wq(struct rcu_head *rcu)
 	wq_free_lockdep(wq);
 	free_percpu(wq->cpu_pwq);
 	free_workqueue_attrs(wq->unbound_attrs);
+	kfree(wq->name);
 	kfree(wq);
 }
 
@@ -4670,9 +4669,10 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 					 unsigned int flags,
 					 int max_active, ...)
 {
-	va_list args;
+	va_list args, args_copy;
 	struct workqueue_struct *wq;
 	struct pool_workqueue *pwq;
+	size_t namelen;
 
 	/*
 	 * Unbound && max_active == 1 used to imply ordered, which is no longer
@@ -4699,8 +4699,16 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 	}
 
 	va_start(args, max_active);
-	vsnprintf(wq->name, sizeof(wq->name), fmt, args);
+	va_copy(args_copy, args);
+	namelen = vsnprintf(NULL, 0, fmt, args) + 1;
+	namelen = (namelen < PAGE_SIZE) ? namelen : PAGE_SIZE;
 	va_end(args);
+	wq->name = kzalloc(namelen, GFP_KERNEL);
+	if (!wq->name)
+		goto err_free_wq;
+
+	vsnprintf(wq->name, namelen, fmt, args_copy);
+	va_end(args_copy);
 
 	max_active = max_active ?: WQ_DFL_ACTIVE;
 	max_active = wq_clamp_max_active(max_active, flags, wq->name);
@@ -4746,6 +4754,7 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 	return wq;
 
 err_unreg_lockdep:
+	kfree(wq->name);
 	wq_unregister_lockdep(wq);
 	wq_free_lockdep(wq);
 err_free_wq:
@@ -5038,7 +5047,7 @@ EXPORT_SYMBOL_GPL(set_worker_desc);
 void print_worker_info(const char *log_lvl, struct task_struct *task)
 {
 	work_func_t *fn = NULL;
-	char name[WQ_NAME_LEN] = { };
+	char *name;
 	char desc[WORKER_DESC_LEN] = { };
 	struct pool_workqueue *pwq = NULL;
 	struct workqueue_struct *wq = NULL;
@@ -5060,7 +5069,7 @@ void print_worker_info(const char *log_lvl, struct task_struct *task)
 	copy_from_kernel_nofault(&fn, &worker->current_func, sizeof(fn));
 	copy_from_kernel_nofault(&pwq, &worker->current_pwq, sizeof(pwq));
 	copy_from_kernel_nofault(&wq, &pwq->wq, sizeof(wq));
-	copy_from_kernel_nofault(name, wq->name, sizeof(name) - 1);
+	copy_from_kernel_nofault(&name, &wq->name, sizeof(wq->name));
 	copy_from_kernel_nofault(desc, worker->desc, sizeof(desc) - 1);
 
 	if (fn || name[0] || desc[0]) {
-- 
2.43.0

Re: [PATCH] workqueue.c: Change workqueue to accept variable length name

[Tejun Heo]

Hello,

On Fri, Dec 15, 2023 at 02:39:54PM -0500, Audra Mitchell wrote:
> Currently we limit the size of the workqueue name to 24 characters due to
> commit ecf6881ff349 ("workqueue: make workqueue->name[] fixed len")
> As device names increase in size a static size for the workqueue name no
> longer satisfies the size requirement, leading to truncated workqueue
> names as we append the device name to the workqueue name. Truncation of
> the workqueue names can cause issues when debugging as each is unique to
> the associated device. Bring back the flexibility of a variable length
> workqueue name to prevent truncation.

I'm not necessarily against it but workqueue code uses that name to
construct rescuer names which are exposed through /proc/PID/comm and so on,
so too long a name can become a nuisance too. Can you give some examples of
the names where 24 char limit is an issue? Can they be shortened?

Thanks.

-- 
tejun

Re: [PATCH] workqueue.c: Change workqueue to accept variable length name

[Tejun Heo]

Hello,

On Fri, Dec 22, 2023 at 10:35:03AM -0500, Audra Mitchell wrote:
> We have one concrete example from a Hitachi block device driver (notice the
> 47a1/47a2 gets
> cut off with the workqueue name):
> 
> Device                              Workqueue Name (24char zero terminated)
> /dev/sd0279b080047a1   xfs-blockgc/sd0279b0800
> /dev/sd0279b080047a2   xfs-blockgc/sd0279b0800

I see, so it's a combination of somewhat lengthy device names and then xfs
adding a prefix to them. Neither is particularly long but the combination
is.

> I can also imagine this issue being present with nvme devices, but the
> request came from Hitachi.
> I believe it would be up to the device driver to determine if the name can
> be shortened and I've
> included Hitachi requester on this email thread.
> 
> Alternatively, we could increase the size of the WQ_NAME_LEN, but it seems
> highly likely we are
> going to butt against the static size again in the future. We previously
> had variable length names
> and it seems (to me) to be the best long term path forward.

Can we just bump the length to 32 and trigger a warning if the requested
name overruns? I want to provide some pressure to limit the length of the
name so that it doesn't get too long over time. If folks bump into it and
can't find a different way to deal with it, we can get bring back the
subject.

Thanks.

-- 
tejun

Re: [PATCH] workqueue.c: Change workqueue to accept variable length name

[Audra Mitchell]

On Sat, Dec 23, 2023 at 02:53:52AM +0900, Tejun Heo wrote:
> Hello,
> 
> On Fri, Dec 22, 2023 at 10:35:03AM -0500, Audra Mitchell wrote:
> > We have one concrete example from a Hitachi block device driver (notice the
> > 47a1/47a2 gets
> > cut off with the workqueue name):
> > 
> > Device                              Workqueue Name (24char zero terminated)
> > /dev/sd0279b080047a1   xfs-blockgc/sd0279b0800
> > /dev/sd0279b080047a2   xfs-blockgc/sd0279b0800
> 
> I see, so it's a combination of somewhat lengthy device names and then xfs
> adding a prefix to them. Neither is particularly long but the combination
> is.
> 
> > I can also imagine this issue being present with nvme devices, but the
> > request came from Hitachi.
> > I believe it would be up to the device driver to determine if the name can
> > be shortened and I've
> > included Hitachi requester on this email thread.
> > 
> > Alternatively, we could increase the size of the WQ_NAME_LEN, but it seems
> > highly likely we are
> > going to butt against the static size again in the future. We previously
> > had variable length names
> > and it seems (to me) to be the best long term path forward.
> 
> Can we just bump the length to 32 and trigger a warning if the requested
> name overruns? I want to provide some pressure to limit the length of the
> name so that it doesn't get too long over time. If folks bump into it and
> can't find a different way to deal with it, we can get bring back the
> subject.

Hey Tejun!

Hope you had a nice holiday. I just got back from a bit of a break and will
work on your suggestions this week. Thanks a bunch for your feedback!

- Audra

[PATCH v2] workqueue.c: Increase workqueue name length

[Audra Mitchell]

Currently we limit the size of the workqueue name to 24 characters due to
commit ecf6881ff349 ("workqueue: make workqueue->name[] fixed len")
Increase the size to 32 characters and print a warning in the event
the requested name is larger than the limit of 32 characters.

Signed-off-by: Audra Mitchell <audra@redhat.com>
---
 kernel/workqueue.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 76e60faed892..cac3b8895c16 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -108,7 +108,7 @@ enum {
 	RESCUER_NICE_LEVEL	= MIN_NICE,
 	HIGHPRI_NICE_LEVEL	= MIN_NICE,
 
-	WQ_NAME_LEN		= 24,
+	WQ_NAME_LEN		= 32,
 };
 
 /*
@@ -4663,9 +4663,10 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 					 unsigned int flags,
 					 int max_active, ...)
 {
-	va_list args;
+	va_list args, args_copy;
 	struct workqueue_struct *wq;
 	struct pool_workqueue *pwq;
+	int len;
 
 	/*
 	 * Unbound && max_active == 1 used to imply ordered, which is no longer
@@ -4692,6 +4693,13 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 	}
 
 	va_start(args, max_active);
+	va_copy(args_copy, args);
+	len = vsnprintf(NULL, 0, fmt, args_copy);
+	WARN(len > WQ_NAME_LEN,
+		"workqueue: wq->name too long (%d). Truncated to WQ_NAME_LEN (%d)\n",
+		len, WQ_NAME_LEN);
+
+	va_end(args_copy);
 	vsnprintf(wq->name, sizeof(wq->name), fmt, args);
 	va_end(args);
 
-- 
2.43.0

Re: [PATCH v2] workqueue.c: Increase workqueue name length

[Rasmus Villemoes]

On 10/01/2024 21.29, Audra Mitchell wrote:

> @@ -4663,9 +4663,10 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
>  					 unsigned int flags,
>  					 int max_active, ...)
>  {
> -	va_list args;
> +	va_list args, args_copy;
>  	struct workqueue_struct *wq;
>  	struct pool_workqueue *pwq;
> +	int len;
>  
>  	/*
>  	 * Unbound && max_active == 1 used to imply ordered, which is no longer
> @@ -4692,6 +4693,13 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
>  	}
>  
>  	va_start(args, max_active);
> +	va_copy(args_copy, args);
> +	len = vsnprintf(NULL, 0, fmt, args_copy);
> +	WARN(len > WQ_NAME_LEN,
> +		"workqueue: wq->name too long (%d). Truncated to WQ_NAME_LEN (%d)\n",
> +		len, WQ_NAME_LEN);
> +
> +	va_end(args_copy);
>  	vsnprintf(wq->name, sizeof(wq->name), fmt, args);

Eh, why not just _not_ throw away the return value from the existing
vsnprintf() and do "len >= sizeof(wq->name)" to know if truncation
happened? There's really no need need to do vsnprintf() twice. (And yes,
you want >=, not >).

Oh, and definitely not WARN,  pr_warn() or pr_warn_once() please.

Rasmus

Re: [PATCH v2] workqueue.c: Increase workqueue name length

[Rafael Aquini]

On Wed, Jan 10, 2024 at 09:47:56PM +0100, Rasmus Villemoes wrote:
> On 10/01/2024 21.29, Audra Mitchell wrote:
> 
> > @@ -4663,9 +4663,10 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
> >  					 unsigned int flags,
> >  					 int max_active, ...)
> >  {
> > -	va_list args;
> > +	va_list args, args_copy;
> >  	struct workqueue_struct *wq;
> >  	struct pool_workqueue *pwq;
> > +	int len;
> >  
> >  	/*
> >  	 * Unbound && max_active == 1 used to imply ordered, which is no longer
> > @@ -4692,6 +4693,13 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
> >  	}
> >  
> >  	va_start(args, max_active);
> > +	va_copy(args_copy, args);
> > +	len = vsnprintf(NULL, 0, fmt, args_copy);
> > +	WARN(len > WQ_NAME_LEN,
> > +		"workqueue: wq->name too long (%d). Truncated to WQ_NAME_LEN (%d)\n",
> > +		len, WQ_NAME_LEN);
> > +
> > +	va_end(args_copy);
> >  	vsnprintf(wq->name, sizeof(wq->name), fmt, args);
> 
> Eh, why not just _not_ throw away the return value from the existing
> vsnprintf() and do "len >= sizeof(wq->name)" to know if truncation
> happened? There's really no need need to do vsnprintf() twice. (And yes,
> you want >=, not >).
>

The extra vsnprintf call is required because the return of the existing 
vsnprintf() is going to be already capped by sizeof(wq->name).
 
> Oh, and definitely not WARN,  pr_warn() or pr_warn_once() please.
> 

Then you lose the ability to figure out what was trying to create the
wq with the inflated name. Also, the _once variants don't seem to do
good here, because alloc_workqueue() can be called by different 
drivers.

-- Rafael

Re: [PATCH v2] workqueue.c: Increase workqueue name length

[Rasmus Villemoes]

On 10/01/2024 22.52, Rafael Aquini wrote:
> On Wed, Jan 10, 2024 at 09:47:56PM +0100, Rasmus Villemoes wrote:
>> On 10/01/2024 21.29, Audra Mitchell wrote:
>>
>>> @@ -4663,9 +4663,10 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
>>>  					 unsigned int flags,
>>>  					 int max_active, ...)
>>>  {
>>> -	va_list args;
>>> +	va_list args, args_copy;
>>>  	struct workqueue_struct *wq;
>>>  	struct pool_workqueue *pwq;
>>> +	int len;
>>>  
>>>  	/*
>>>  	 * Unbound && max_active == 1 used to imply ordered, which is no longer
>>> @@ -4692,6 +4693,13 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
>>>  	}
>>>  
>>>  	va_start(args, max_active);
>>> +	va_copy(args_copy, args);
>>> +	len = vsnprintf(NULL, 0, fmt, args_copy);
>>> +	WARN(len > WQ_NAME_LEN,
>>> +		"workqueue: wq->name too long (%d). Truncated to WQ_NAME_LEN (%d)\n",
>>> +		len, WQ_NAME_LEN);
>>> +
>>> +	va_end(args_copy);
>>>  	vsnprintf(wq->name, sizeof(wq->name), fmt, args);
>>
>> Eh, why not just _not_ throw away the return value from the existing
>> vsnprintf() and do "len >= sizeof(wq->name)" to know if truncation
>> happened? There's really no need need to do vsnprintf() twice. (And yes,
>> you want >=, not >).
>>
> 
> The extra vsnprintf call is required because the return of the existing 
> vsnprintf() is going to be already capped by sizeof(wq->name).

No, it is not. vsnprintf() returns the length of the would-be-created
string if the buffer was big enough. That is independent of whether one
does a dummy NULL,0 call or just calls it with a real, but possibly too
small, buffer.

This is true for userspace (as required by posix) as well as the kernel
implementation of vsnprintf(). What makes you think otherwise?

The kernel _also_ happens to have a non-standardized function called
vscnprintf (note the c) which returns the possibly-truncated result. But
that's irrelevant here.

>> Oh, and definitely not WARN,  pr_warn() or pr_warn_once() please.
>>
> 
> Then you lose the ability to figure out what was trying to create the
> wq with the inflated name. Also, the _once variants don't seem to do
> good here, because alloc_workqueue() can be called by different 
> drivers.

I assume that whatever creates the wq will do so on every boot, and the
name is most likely some fixed thing. So you're essentially setting up
some configurations to do a WARN on every single boot, not to mention
that for some machines that implies a panic... It really is not
something that warrants a WARN.

As for figuring out what caused that too-long name, well, I'd hope that
the 31 meaningful bytes that did get produced would provide a
sufficiently good hint.

Rasmus

Re: [PATCH v2] workqueue.c: Increase workqueue name length

[Rafael Aquini]

On Wed, Jan 10, 2024 at 11:06:22PM +0100, Rasmus Villemoes wrote:
> On 10/01/2024 22.52, Rafael Aquini wrote:
> > On Wed, Jan 10, 2024 at 09:47:56PM +0100, Rasmus Villemoes wrote:
> >> On 10/01/2024 21.29, Audra Mitchell wrote:
> >>
> >>> @@ -4663,9 +4663,10 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
> >>>  					 unsigned int flags,
> >>>  					 int max_active, ...)
> >>>  {
> >>> -	va_list args;
> >>> +	va_list args, args_copy;
> >>>  	struct workqueue_struct *wq;
> >>>  	struct pool_workqueue *pwq;
> >>> +	int len;
> >>>  
> >>>  	/*
> >>>  	 * Unbound && max_active == 1 used to imply ordered, which is no longer
> >>> @@ -4692,6 +4693,13 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
> >>>  	}
> >>>  
> >>>  	va_start(args, max_active);
> >>> +	va_copy(args_copy, args);
> >>> +	len = vsnprintf(NULL, 0, fmt, args_copy);
> >>> +	WARN(len > WQ_NAME_LEN,
> >>> +		"workqueue: wq->name too long (%d). Truncated to WQ_NAME_LEN (%d)\n",
> >>> +		len, WQ_NAME_LEN);
> >>> +
> >>> +	va_end(args_copy);
> >>>  	vsnprintf(wq->name, sizeof(wq->name), fmt, args);
> >>
> >> Eh, why not just _not_ throw away the return value from the existing
> >> vsnprintf() and do "len >= sizeof(wq->name)" to know if truncation
> >> happened? There's really no need need to do vsnprintf() twice. (And yes,
> >> you want >=, not >).
> >>
> > 
> > The extra vsnprintf call is required because the return of the existing 
> > vsnprintf() is going to be already capped by sizeof(wq->name).
> 
> No, it is not. vsnprintf() returns the length of the would-be-created
> string if the buffer was big enough. That is independent of whether one
> does a dummy NULL,0 call or just calls it with a real, but possibly too
> small, buffer.
> 
> This is true for userspace (as required by posix) as well as the kernel
> implementation of vsnprintf(). What makes you think otherwise?
>

this snippet from PRINTF(3) man page

RETURN VALUE
       Upon successful return, these functions return the number of characters 
       printed (excluding the null byte used to end output to strings).




 
> The kernel _also_ happens to have a non-standardized function called
> vscnprintf (note the c) which returns the possibly-truncated result. But
> that's irrelevant here.
> 
> >> Oh, and definitely not WARN,  pr_warn() or pr_warn_once() please.
> >>
> > 
> > Then you lose the ability to figure out what was trying to create the
> > wq with the inflated name. Also, the _once variants don't seem to do
> > good here, because alloc_workqueue() can be called by different 
> > drivers.
> 
> I assume that whatever creates the wq will do so on every boot, and the
> name is most likely some fixed thing. So you're essentially setting up
> some configurations to do a WARN on every single boot, not to mention
> that for some machines that implies a panic... It really is not
> something that warrants a WARN.
> 
> As for figuring out what caused that too-long name, well, I'd hope that
> the 31 meaningful bytes that did get produced would provide a
> sufficiently good hint.
> 
> Rasmus
> 

Re: [PATCH v2] workqueue.c: Increase workqueue name length

[Rafael Aquini]

On Wed, Jan 10, 2024 at 05:31:49PM -0500, Rafael Aquini wrote:
> On Wed, Jan 10, 2024 at 11:06:22PM +0100, Rasmus Villemoes wrote:
> > On 10/01/2024 22.52, Rafael Aquini wrote:
> > > On Wed, Jan 10, 2024 at 09:47:56PM +0100, Rasmus Villemoes wrote:
> > >> On 10/01/2024 21.29, Audra Mitchell wrote:
> > >>
> > >>> @@ -4663,9 +4663,10 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
> > >>>  					 unsigned int flags,
> > >>>  					 int max_active, ...)
> > >>>  {
> > >>> -	va_list args;
> > >>> +	va_list args, args_copy;
> > >>>  	struct workqueue_struct *wq;
> > >>>  	struct pool_workqueue *pwq;
> > >>> +	int len;
> > >>>  
> > >>>  	/*
> > >>>  	 * Unbound && max_active == 1 used to imply ordered, which is no longer
> > >>> @@ -4692,6 +4693,13 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
> > >>>  	}
> > >>>  
> > >>>  	va_start(args, max_active);
> > >>> +	va_copy(args_copy, args);
> > >>> +	len = vsnprintf(NULL, 0, fmt, args_copy);
> > >>> +	WARN(len > WQ_NAME_LEN,
> > >>> +		"workqueue: wq->name too long (%d). Truncated to WQ_NAME_LEN (%d)\n",
> > >>> +		len, WQ_NAME_LEN);
> > >>> +
> > >>> +	va_end(args_copy);
> > >>>  	vsnprintf(wq->name, sizeof(wq->name), fmt, args);
> > >>
> > >> Eh, why not just _not_ throw away the return value from the existing
> > >> vsnprintf() and do "len >= sizeof(wq->name)" to know if truncation
> > >> happened? There's really no need need to do vsnprintf() twice. (And yes,
> > >> you want >=, not >).
> > >>
> > > 
> > > The extra vsnprintf call is required because the return of the existing 
> > > vsnprintf() is going to be already capped by sizeof(wq->name).
> > 
> > No, it is not. vsnprintf() returns the length of the would-be-created
> > string if the buffer was big enough. That is independent of whether one
> > does a dummy NULL,0 call or just calls it with a real, but possibly too
> > small, buffer.
> > 
> > This is true for userspace (as required by posix) as well as the kernel
> > implementation of vsnprintf(). What makes you think otherwise?
> >
> 
> this snippet from PRINTF(3) man page
> 
> RETURN VALUE
>        Upon successful return, these functions return the number of characters 
>        printed (excluding the null byte used to end output to strings).
>

Perhaps the man page should be amended to indicate vsnprintf returns the
number of characters that would have been printed if the buffer size 
were unlimited.

We based the assumption of kernel's vsnprintf behavior out of the 
documented POSIX behavior as stated on the man page.

Re: [PATCH v2] workqueue.c: Increase workqueue name length

[Rasmus Villemoes]

On 10/01/2024 23.45, Rafael Aquini wrote:
> On Wed, Jan 10, 2024 at 05:31:49PM -0500, Rafael Aquini wrote:

>> this snippet from PRINTF(3) man page
>>
>> RETURN VALUE
>>        Upon successful return, these functions return the number of characters 
>>        printed (excluding the null byte used to end output to strings).
>>
> 
> Perhaps the man page should be amended to indicate vsnprintf returns the
> number of characters that would have been printed if the buffer size 
> were unlimited.
> 
> We based the assumption of kernel's vsnprintf behavior out of the 
> documented POSIX behavior as stated on the man page.

That's not at all the "documented POSIX behaviour". There's also no
reason to take that from badly (re)worded man pages when the horse's
mouth is right here:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/snprintf.html
(and for vsnprintf,
https://pubs.opengroup.org/onlinepubs/9699919799/functions/vfprintf.html
, but that explicitly says it's the same as snprintf except for how the
varargs are passed).

Rasmus

Re: [PATCH v2] workqueue.c: Increase workqueue name length

[Rasmus Villemoes]

On 10/01/2024 23.31, Rafael Aquini wrote:
> On Wed, Jan 10, 2024 at 11:06:22PM +0100, Rasmus Villemoes wrote:
>> On 10/01/2024 22.52, Rafael Aquini wrote:

>>> The extra vsnprintf call is required because the return of the existing 
>>> vsnprintf() is going to be already capped by sizeof(wq->name).
>>
>> No, it is not. vsnprintf() returns the length of the would-be-created
>> string if the buffer was big enough. That is independent of whether one
>> does a dummy NULL,0 call or just calls it with a real, but possibly too
>> small, buffer.
>>
>> This is true for userspace (as required by posix) as well as the kernel
>> implementation of vsnprintf(). What makes you think otherwise?
>>
> 
> this snippet from PRINTF(3) man page
> 
> RETURN VALUE
>        Upon successful return, these functions return the number of characters 
>        printed (excluding the null byte used to end output to strings).
> 

Assuming we have the same man pages installed, try reading the very next
paragraph:

 The functions snprintf() and vsnprintf() do not write  more  than  size
 bytes  (including the terminating null byte ('\0')).  If the output was
 truncated due to this limit, then the return value  is  the  number  of
 characters  (excluding the terminating null byte) which would have been
 written to the final string if enough space had been available.   Thus,
 a  return  value  of  size or more means that the output was truncated.

How else would you even expect the vsnprintf(NULL, 0, ...) thing to work?

Re: [PATCH v2] workqueue.c: Increase workqueue name length

[Rafael Aquini]

On Wed, Jan 10, 2024 at 11:52:38PM +0100, Rasmus Villemoes wrote:
> On 10/01/2024 23.31, Rafael Aquini wrote:
> > On Wed, Jan 10, 2024 at 11:06:22PM +0100, Rasmus Villemoes wrote:
> >> On 10/01/2024 22.52, Rafael Aquini wrote:
> 
> >>> The extra vsnprintf call is required because the return of the existing 
> >>> vsnprintf() is going to be already capped by sizeof(wq->name).
> >>
> >> No, it is not. vsnprintf() returns the length of the would-be-created
> >> string if the buffer was big enough. That is independent of whether one
> >> does a dummy NULL,0 call or just calls it with a real, but possibly too
> >> small, buffer.
> >>
> >> This is true for userspace (as required by posix) as well as the kernel
> >> implementation of vsnprintf(). What makes you think otherwise?
> >>
> > 
> > this snippet from PRINTF(3) man page
> > 
> > RETURN VALUE
> >        Upon successful return, these functions return the number of characters 
> >        printed (excluding the null byte used to end output to strings).
> > 
> 
> Assuming we have the same man pages installed, try reading the very next
> paragraph:
> 
>  The functions snprintf() and vsnprintf() do not write  more  than  size
>  bytes  (including the terminating null byte ('\0')).  If the output was
>  truncated due to this limit, then the return value  is  the  number  of
>  characters  (excluding the terminating null byte) which would have been
>  written to the final string if enough space had been available.   Thus,
>  a  return  value  of  size or more means that the output was truncated.
> 
> How else would you even expect the vsnprintf(NULL, 0, ...) thing to work?
>

OK, that's my bad! Sorry for the noise.
-- Rafael 

[PATCH v3] workqueue.c: Increase workqueue name length

[Audra Mitchell]

Currently we limit the size of the workqueue name to 24 characters due to
commit ecf6881ff349 ("workqueue: make workqueue->name[] fixed len")
Increase the size to 32 characters and print a warning in the event
the requested name is larger than the limit of 32 characters.

Signed-off-by: Audra Mitchell <audra@redhat.com>
---
 kernel/workqueue.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 76e60faed892..8d9dec14b9bb 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -108,7 +108,7 @@ enum {
 	RESCUER_NICE_LEVEL	= MIN_NICE,
 	HIGHPRI_NICE_LEVEL	= MIN_NICE,
 
-	WQ_NAME_LEN		= 24,
+	WQ_NAME_LEN		= 32,
 };
 
 /*
@@ -4666,6 +4666,7 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 	va_list args;
 	struct workqueue_struct *wq;
 	struct pool_workqueue *pwq;
+	int len;
 
 	/*
 	 * Unbound && max_active == 1 used to imply ordered, which is no longer
@@ -4692,9 +4693,12 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 	}
 
 	va_start(args, max_active);
-	vsnprintf(wq->name, sizeof(wq->name), fmt, args);
+	len = vsnprintf(wq->name, sizeof(wq->name), fmt, args);
 	va_end(args);
 
+	if (len >= WQ_NAME_LEN)
+		pr_warn_once("workqueue: name exceeds WQ_NAME_LEN. Truncating to: %s\n", wq->name);
+
 	max_active = max_active ?: WQ_DFL_ACTIVE;
 	max_active = wq_clamp_max_active(max_active, flags, wq->name);
 
-- 
2.43.0

Re: [PATCH v3] workqueue.c: Increase workqueue name length

[Tejun Heo]

On Mon, Jan 15, 2024 at 12:08:22PM -0500, Audra Mitchell wrote:
> Currently we limit the size of the workqueue name to 24 characters due to
> commit ecf6881ff349 ("workqueue: make workqueue->name[] fixed len")
> Increase the size to 32 characters and print a warning in the event
> the requested name is larger than the limit of 32 characters.
> 
> Signed-off-by: Audra Mitchell <audra@redhat.com>

Applied to wq/for-6.9.

Thanks.

-- 
tejun

Re: [PATCH v3] workqueue.c: Increase workqueue name length

[Audra Mitchell]

On Tue, Jan 16, 2024 at 08:31:45AM -1000, Tejun Heo wrote:
> On Mon, Jan 15, 2024 at 12:08:22PM -0500, Audra Mitchell wrote:
> > Currently we limit the size of the workqueue name to 24 characters due to
> > commit ecf6881ff349 ("workqueue: make workqueue->name[] fixed len")
> > Increase the size to 32 characters and print a warning in the event
> > the requested name is larger than the limit of 32 characters.
> > 
> > Signed-off-by: Audra Mitchell <audra@redhat.com>
> 
> Applied to wq/for-6.9.
> 
> Thanks.

Hey Tejun (and all others)

Thank you for the response. May I humbly mention that I did find the following
while testing my patch:

[    0.120298] workqueue: name exceeds WQ_NAME_LEN (32 chars). Truncating to: events_freezable_power_efficien

In an effort to be thorough, would you like me to submit a patch shortening
this? Perhaps to "events_freezable_pwr_efficient"?

To be clear, I am not pushing the change, however, I do want to make sure that
the changes being submitted are not causing additional clutter. 

Thanks!

Re: [PATCH v3] workqueue.c: Increase workqueue name length

[Tejun Heo]

Hello,

On Wed, Jan 17, 2024 at 09:40:58AM -0500, Audra Mitchell wrote:
> Thank you for the response. May I humbly mention that I did find the following
> while testing my patch:
> 
> [    0.120298] workqueue: name exceeds WQ_NAME_LEN (32 chars). Truncating to: events_freezable_power_efficien
> 
> In an effort to be thorough, would you like me to submit a patch shortening
> this? Perhaps to "events_freezable_pwr_efficient"?
> 
> To be clear, I am not pushing the change, however, I do want to make sure that
> the changes being submitted are not causing additional clutter. 

Oh yeah, please go ahead.

Thanks.

-- 
tejun

Re: [PATCH v3] workqueue.c: Increase workqueue name length

[Audra Mitchell]

On Wed, Jan 17, 2024 at 07:09:02AM -1000, Tejun Heo wrote:
> Hello,
> 
> On Wed, Jan 17, 2024 at 09:40:58AM -0500, Audra Mitchell wrote:
> > Thank you for the response. May I humbly mention that I did find the following
> > while testing my patch:
> > 
> > [    0.120298] workqueue: name exceeds WQ_NAME_LEN (32 chars). Truncating to: events_freezable_power_efficien
> > 
> > In an effort to be thorough, would you like me to submit a patch shortening
> > this? Perhaps to "events_freezable_pwr_efficient"?
> > 
> > To be clear, I am not pushing the change, however, I do want to make sure that
> > the changes being submitted are not causing additional clutter. 
> 
> Oh yeah, please go ahead.
> 
> Thanks.

Hey Tejun,

Do you want this as a stand alone patch or do you want me to re-submit both
patches as a series?

Thanks

Re: [PATCH v3] workqueue.c: Increase workqueue name length

[Tejun Heo]

On Fri, Jan 19, 2024 at 03:30:54PM -0500, Audra Mitchell wrote:
> Do you want this as a stand alone patch or do you want me to re-submit both
> patches as a series?

A separate patch would be great.

Thanks.

-- 
tejun