kernel/workqueue.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
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.0On 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
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
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
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
>
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?
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
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.
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
© 2016 - 2025 Red Hat, Inc.