From: FUJITA Tomonori <fujita.tomonori@gmail.com>

Add __might_sleep_precision(), Rust friendly version of
__might_sleep(), which takes a pointer to a string with the length
instead of a null-terminated string.

Rust's core::panic::Location::file(), which gives the file name of a
caller, doesn't provide a null-terminated
string. __might_sleep_precision() uses a precision specifier in the
printk format, which specifies the length of a string; a string
doesn't need to be a null-terminated.

Modify __might_sleep() to call __might_sleep_precision() but the
impact should be negligible. When printing the error (sleeping
function called from invalid context), the precision string format is
used instead of the simple string format; the precision specifies the
the maximum length of the displayed string.

Note that Location::file() providing a null-terminated string for
better C interoperability is under discussion [1].

[1]: https://github.com/rust-lang/libs-team/issues/466

Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Link: https://lore.kernel.org/r/20250410225623.152616-2-fujita.tomonori@gmail.com
---
 include/linux/kernel.h |  2 ++
 kernel/sched/core.c    | 62 ++++++++++++++++++++++++++++--------------
 2 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index be2e8c0a187e..086ee1dc447e 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -87,6 +87,7 @@ extern int dynamic_might_resched(void);
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 extern void __might_resched(const char *file, int line, unsigned int offsets);
 extern void __might_sleep(const char *file, int line);
+extern void __might_sleep_precision(const char *file, int len, int line);
 extern void __cant_sleep(const char *file, int line, int preempt_offset);
 extern void __cant_migrate(const char *file, int line);
 
@@ -145,6 +146,7 @@ extern void __cant_migrate(const char *file, int line);
   static inline void __might_resched(const char *file, int line,
 				     unsigned int offsets) { }
 static inline void __might_sleep(const char *file, int line) { }
+static inline void __might_sleep_precision(const char *file, int len, int line) { }
 # define might_sleep() do { might_resched(); } while (0)
 # define cant_sleep() do { } while (0)
 # define cant_migrate()		do { } while (0)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 79692f85643f..496bd462d29e 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8732,24 +8732,6 @@ void __init sched_init(void)
 
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 
-void __might_sleep(const char *file, int line)
-{
-	unsigned int state = get_current_state();
-	/*
-	 * Blocking primitives will set (and therefore destroy) current->state,
-	 * since we will exit with TASK_RUNNING make sure we enter with it,
-	 * otherwise we will destroy state.
-	 */
-	WARN_ONCE(state != TASK_RUNNING && current->task_state_change,
-			"do not call blocking ops when !TASK_RUNNING; "
-			"state=%x set at [<%p>] %pS\n", state,
-			(void *)current->task_state_change,
-			(void *)current->task_state_change);
-
-	__might_resched(file, line, 0);
-}
-EXPORT_SYMBOL(__might_sleep);
-
 static void print_preempt_disable_ip(int preempt_offset, unsigned long ip)
 {
 	if (!IS_ENABLED(CONFIG_DEBUG_PREEMPT))
@@ -8771,7 +8753,8 @@ static inline bool resched_offsets_ok(unsigned int offsets)
 	return nested == offsets;
 }
 
-void __might_resched(const char *file, int line, unsigned int offsets)
+static void __might_resched_precision(const char *file, int file_len, int line,
+				      unsigned int offsets)
 {
 	/* Ratelimiting timestamp: */
 	static unsigned long prev_jiffy;
@@ -8794,8 +8777,8 @@ void __might_resched(const char *file, int line, unsigned int offsets)
 	/* Save this before calling printk(), since that will clobber it: */
 	preempt_disable_ip = get_preempt_disable_ip(current);
 
-	pr_err("BUG: sleeping function called from invalid context at %s:%d\n",
-	       file, line);
+	pr_err("BUG: sleeping function called from invalid context at %.*s:%d\n",
+	       file_len, file, line);
 	pr_err("in_atomic(): %d, irqs_disabled(): %d, non_block: %d, pid: %d, name: %s\n",
 	       in_atomic(), irqs_disabled(), current->non_block_count,
 	       current->pid, current->comm);
@@ -8820,8 +8803,45 @@ void __might_resched(const char *file, int line, unsigned int offsets)
 	dump_stack();
 	add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
 }
+
+/*
+ * The precision in vsnprintf() specifies the maximum length of the
+ * displayed string. The precision needs to be larger than the actual
+ * length of the string, so a sufficiently large value should be used
+ * for the filename length.
+ */
+#define MAX_FILENAME_LEN (1<<14)
+
+void __might_resched(const char *file, int line, unsigned int offsets)
+{
+	__might_resched_precision(file, MAX_FILENAME_LEN, line, offsets);
+}
 EXPORT_SYMBOL(__might_resched);
 
+void __might_sleep_precision(const char *file, int len, int line)
+{
+	unsigned int state = get_current_state();
+	/*
+	 * Blocking primitives will set (and therefore destroy) current->state,
+	 * since we will exit with TASK_RUNNING make sure we enter with it,
+	 * otherwise we will destroy state.
+	 */
+	WARN_ONCE(state != TASK_RUNNING && current->task_state_change,
+			"do not call blocking ops when !TASK_RUNNING; "
+			"state=%x set at [<%p>] %pS\n", state,
+			(void *)current->task_state_change,
+			(void *)current->task_state_change);
+
+	__might_resched_precision(file, len, line, 0);
+}
+EXPORT_SYMBOL_GPL(__might_sleep_precision);
+
+void __might_sleep(const char *file, int line)
+{
+	__might_sleep_precision(file, MAX_FILENAME_LEN, line);
+}
+EXPORT_SYMBOL(__might_sleep);
+
 void __cant_sleep(const char *file, int line, int preempt_offset)
 {
 	static unsigned long prev_jiffy;
-- 
2.39.5 (Apple Git-154)

* Boqun Feng <boqun.feng@gmail.com> wrote:

> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Add __might_sleep_precision(), Rust friendly version of
> __might_sleep(), which takes a pointer to a string with the length
> instead of a null-terminated string.
> 
> Rust's core::panic::Location::file(), which gives the file name of a
> caller, doesn't provide a null-terminated
> string. __might_sleep_precision() uses a precision specifier in the
> printk format, which specifies the length of a string; a string
> doesn't need to be a null-terminated.
> 
> Modify __might_sleep() to call __might_sleep_precision() but the
> impact should be negligible. When printing the error (sleeping
> function called from invalid context), the precision string format is
> used instead of the simple string format; the precision specifies the
> the maximum length of the displayed string.
> 
> Note that Location::file() providing a null-terminated string for
> better C interoperability is under discussion [1].
> 
> [1]: https://github.com/rust-lang/libs-team/issues/466
> 
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Link: https://lore.kernel.org/r/20250410225623.152616-2-fujita.tomonori@gmail.com
> ---
>  include/linux/kernel.h |  2 ++
>  kernel/sched/core.c    | 62 ++++++++++++++++++++++++++++--------------
>  2 files changed, 43 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index be2e8c0a187e..086ee1dc447e 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -87,6 +87,7 @@ extern int dynamic_might_resched(void);
>  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
>  extern void __might_resched(const char *file, int line, unsigned int offsets);
>  extern void __might_sleep(const char *file, int line);
> +extern void __might_sleep_precision(const char *file, int len, int line);

Ugh.

Firstly, '_precision' is really ambiguous in this context and suggests 
'precise sleep' or something like that, which this is not about at all. 
So the naming here is all sorts of bad already.

But more importantly, this is really a Rust problem. Does Rust really 
have no NUL-terminated strings? It should hide them in shame and 
construct proper, robust strings, instead of spreading this disease to 
the rest of the kernel, IMHO ...

Rust is supposed to be about increased security, right? How does extra, 
nonsensical complexity for simple concepts such as strings achieve 
that? If the Rust runtime wants to hook into debug facilities of the 
Linux kernel then I have bad news: almost all strings used by kernel 
debugging facilities are NUL-terminated.

So I really don't like this patch. Is there no other way to do this?

Thanks,

	Ingo

On Fri, May 09, 2025 at 08:00:32AM +0200, Ingo Molnar wrote:
> 
> * Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > From: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > 
> > Add __might_sleep_precision(), Rust friendly version of
> > __might_sleep(), which takes a pointer to a string with the length
> > instead of a null-terminated string.
> > 
> > Rust's core::panic::Location::file(), which gives the file name of a
> > caller, doesn't provide a null-terminated
> > string. __might_sleep_precision() uses a precision specifier in the
> > printk format, which specifies the length of a string; a string
> > doesn't need to be a null-terminated.
> > 
> > Modify __might_sleep() to call __might_sleep_precision() but the
> > impact should be negligible. When printing the error (sleeping
> > function called from invalid context), the precision string format is
> > used instead of the simple string format; the precision specifies the
> > the maximum length of the displayed string.
> > 
> > Note that Location::file() providing a null-terminated string for
> > better C interoperability is under discussion [1].
> > 
> > [1]: https://github.com/rust-lang/libs-team/issues/466
> > 
> > Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > Link: https://lore.kernel.org/r/20250410225623.152616-2-fujita.tomonori@gmail.com
> > ---
> >  include/linux/kernel.h |  2 ++
> >  kernel/sched/core.c    | 62 ++++++++++++++++++++++++++++--------------
> >  2 files changed, 43 insertions(+), 21 deletions(-)
> > 
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index be2e8c0a187e..086ee1dc447e 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -87,6 +87,7 @@ extern int dynamic_might_resched(void);
> >  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
> >  extern void __might_resched(const char *file, int line, unsigned int offsets);
> >  extern void __might_sleep(const char *file, int line);
> > +extern void __might_sleep_precision(const char *file, int len, int line);
> 
> Ugh.
> 
> Firstly, '_precision' is really ambiguous in this context and suggests 
> 'precise sleep' or something like that, which this is not about at all. 
> So the naming here is all sorts of bad already.
> 
> But more importantly, this is really a Rust problem. Does Rust really 
> have no NUL-terminated strings? It should hide them in shame and 
> construct proper, robust strings, instead of spreading this disease to 
> the rest of the kernel, IMHO ...

Rust does have NUL-terminated strings, but they aren't the default. In
most circumstances, obtaining a NUL-terminated string is possible, but
we can't do it in this particular case.

Specifically, it's because we're relying on the #[track_caller] language
feature. When this annotation is placed on a function, it implicitly
adds an extra hidden argument to the function signature containing a
pointer to a location struct that holds the file, line, and column of
the caller. This works recursively until it hits a function without the
annotation, so code like this:

#[track_caller]
fn schedule() {
     might_sleep();
     // Call into C implementation of schedule.
     unsafe { bindings::schedule() };
}

would report a line number in the *caller* of schedule() rather than
just reporting the line number inside the schedule() function.

The problem is that the location struct is generated by the compiler,
and we don't have any control over how its generated. Unfortunately, the
compiler does not place a NUL terminator in the file name.

> Rust is supposed to be about increased security, right? How does extra, 
> nonsensical complexity for simple concepts such as strings achieve 
> that? If the Rust runtime wants to hook into debug facilities of the 
> Linux kernel then I have bad news: almost all strings used by kernel 
> debugging facilities are NUL-terminated.
> 
> So I really don't like this patch. Is there no other way to do this?

I filed a proposal to add a NUL-terminator to the file names used by
rustc-generated location structs:

https://github.com/rust-lang/libs-team/issues/466

But I have not had success with landing it, unfortunately.

Alice

Ingo Molnar <mingo@kernel.org> writes:

> * Boqun Feng <boqun.feng@gmail.com> wrote:
>
>> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> 
>> Add __might_sleep_precision(), Rust friendly version of
>> __might_sleep(), which takes a pointer to a string with the length
>> instead of a null-terminated string.
>> 
>> Rust's core::panic::Location::file(), which gives the file name of a
>> caller, doesn't provide a null-terminated
>> string. __might_sleep_precision() uses a precision specifier in the
>> printk format, which specifies the length of a string; a string
>> doesn't need to be a null-terminated.
>> 
>> Modify __might_sleep() to call __might_sleep_precision() but the
>> impact should be negligible. When printing the error (sleeping
>> function called from invalid context), the precision string format is
>> used instead of the simple string format; the precision specifies the
>> the maximum length of the displayed string.
>> 
>> Note that Location::file() providing a null-terminated string for
>> better C interoperability is under discussion [1].
>> 
>> [1]: https://github.com/rust-lang/libs-team/issues/466
>> 
>> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> Link: https://lore.kernel.org/r/20250410225623.152616-2-fujita.tomonori@gmail.com
>> ---
>>  include/linux/kernel.h |  2 ++
>>  kernel/sched/core.c    | 62 ++++++++++++++++++++++++++++--------------
>>  2 files changed, 43 insertions(+), 21 deletions(-)
>> 
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index be2e8c0a187e..086ee1dc447e 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -87,6 +87,7 @@ extern int dynamic_might_resched(void);
>>  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
>>  extern void __might_resched(const char *file, int line, unsigned int offsets);
>>  extern void __might_sleep(const char *file, int line);
>> +extern void __might_sleep_precision(const char *file, int len, int line);
>
> Ugh.
>
> Firstly, '_precision' is really ambiguous in this context and suggests 
> 'precise sleep' or something like that, which this is not about at all. 
> So the naming here is all sorts of bad already.
>
> But more importantly, this is really a Rust problem. Does Rust really 
> have no NUL-terminated strings? It should hide them in shame and 
> construct proper, robust strings, instead of spreading this disease to 
> the rest of the kernel, IMHO ...

Not to discuss this particular patch or language interop in general, but
I am curious why you think that null terminated strings are more robust
than pointer + length strings? From the paragraph below, it seems you
also believe that pointer + length is worse for security. Why is that?


Best regards,
Andreas Hindborg


>
> Rust is supposed to be about increased security, right? How does extra, 
> nonsensical complexity for simple concepts such as strings achieve 
> that? If the Rust runtime wants to hook into debug facilities of the 
> Linux kernel then I have bad news: almost all strings used by kernel 
> debugging facilities are NUL-terminated.
>
> So I really don't like this patch. Is there no other way to do this?
>
> Thanks,
>
> 	Ingo

On Fri, May 09, 2025 at 08:00:32AM +0200, Ingo Molnar wrote:
> 
> * Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > From: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > 
> > Add __might_sleep_precision(), Rust friendly version of
> > __might_sleep(), which takes a pointer to a string with the length
> > instead of a null-terminated string.
> > 
> > Rust's core::panic::Location::file(), which gives the file name of a
> > caller, doesn't provide a null-terminated
> > string. __might_sleep_precision() uses a precision specifier in the
> > printk format, which specifies the length of a string; a string
> > doesn't need to be a null-terminated.
> > 
> > Modify __might_sleep() to call __might_sleep_precision() but the
> > impact should be negligible. When printing the error (sleeping
> > function called from invalid context), the precision string format is
> > used instead of the simple string format; the precision specifies the
> > the maximum length of the displayed string.
> > 
> > Note that Location::file() providing a null-terminated string for
> > better C interoperability is under discussion [1].
> > 
> > [1]: https://github.com/rust-lang/libs-team/issues/466
> > 
> > Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > Link: https://lore.kernel.org/r/20250410225623.152616-2-fujita.tomonori@gmail.com
> > ---
> >  include/linux/kernel.h |  2 ++
> >  kernel/sched/core.c    | 62 ++++++++++++++++++++++++++++--------------
> >  2 files changed, 43 insertions(+), 21 deletions(-)
> > 
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index be2e8c0a187e..086ee1dc447e 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -87,6 +87,7 @@ extern int dynamic_might_resched(void);
> >  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
> >  extern void __might_resched(const char *file, int line, unsigned int offsets);
> >  extern void __might_sleep(const char *file, int line);
> > +extern void __might_sleep_precision(const char *file, int len, int line);
> 
> Ugh.
> 
> Firstly, '_precision' is really ambiguous in this context and suggests 
> 'precise sleep' or something like that, which this is not about at all. 
> So the naming here is all sorts of bad already.
> 

I accept this is not a good naming.

> But more importantly, this is really a Rust problem. Does Rust really 
> have no NUL-terminated strings? It should hide them in shame and 

You can create NUL-terminated strings in Rust of course, but in this
case, because we want to use the "#[trace_caller]" attribute [1], which
allows might_sleep() in Rust to be defined as a function, and can use
Location::caller() to get the caller file and line number information,
and `Location` type yet doesn't return a Nul-terminated string literal,
so we have to work this around.

> construct proper, robust strings, instead of spreading this disease to 
> the rest of the kernel, IMHO ...
> 
> Rust is supposed to be about increased security, right? How does extra, 
> nonsensical complexity for simple concepts such as strings achieve 
> that? If the Rust runtime wants to hook into debug facilities of the 
> Linux kernel then I have bad news: almost all strings used by kernel 
> debugging facilities are NUL-terminated.

This is more of a special case because `Location` is used (i.e. file
name is the string literal). For things like user-defined string, we use
the macro c_str!(), which generates NUL-terminated strings. For example,
lockdep class names.

> 
> So I really don't like this patch. Is there no other way to do this?
> 

There's a `c_str` [2] macro which could generates a NUL-terminated
string, but using that will requires might_sleep() defined as a macro as
well. Given that might_sleep() is the user interface that most users
will use, and how it handles string literal for file names is an
implementation detail, so I figured it's better we resolve in the
current way.

[1]: https://rustc-dev-guide.rust-lang.org/backend/implicit-caller-location.html
[2]: https://rust.docs.kernel.org/kernel/macro.c_str.html

Regards,
Boqun

> Thanks,
> 
> 	Ingo

On Fri, May 09, 2025 at 12:19:03AM -0700, Boqun Feng wrote:
> On Fri, May 09, 2025 at 08:00:32AM +0200, Ingo Molnar wrote:
> > 
> > * Boqun Feng <boqun.feng@gmail.com> wrote:
> > 
> > > From: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > 
> > > Add __might_sleep_precision(), Rust friendly version of
> > > __might_sleep(), which takes a pointer to a string with the length
> > > instead of a null-terminated string.
> > > 
> > > Rust's core::panic::Location::file(), which gives the file name of a
> > > caller, doesn't provide a null-terminated
> > > string. __might_sleep_precision() uses a precision specifier in the
> > > printk format, which specifies the length of a string; a string
> > > doesn't need to be a null-terminated.
> > > 
> > > Modify __might_sleep() to call __might_sleep_precision() but the
> > > impact should be negligible. When printing the error (sleeping
> > > function called from invalid context), the precision string format is
> > > used instead of the simple string format; the precision specifies the
> > > the maximum length of the displayed string.
> > > 
> > > Note that Location::file() providing a null-terminated string for
> > > better C interoperability is under discussion [1].
> > > 
> > > [1]: https://github.com/rust-lang/libs-team/issues/466
> > > 
> > > Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> > > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > Link: https://lore.kernel.org/r/20250410225623.152616-2-fujita.tomonori@gmail.com
> > > ---
> > >  include/linux/kernel.h |  2 ++
> > >  kernel/sched/core.c    | 62 ++++++++++++++++++++++++++++--------------
> > >  2 files changed, 43 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > > index be2e8c0a187e..086ee1dc447e 100644
> > > --- a/include/linux/kernel.h
> > > +++ b/include/linux/kernel.h
> > > @@ -87,6 +87,7 @@ extern int dynamic_might_resched(void);
> > >  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
> > >  extern void __might_resched(const char *file, int line, unsigned int offsets);
> > >  extern void __might_sleep(const char *file, int line);
> > > +extern void __might_sleep_precision(const char *file, int len, int line);
> > 
> > Ugh.
> > 
> > Firstly, '_precision' is really ambiguous in this context and suggests 
> > 'precise sleep' or something like that, which this is not about at all. 
> > So the naming here is all sorts of bad already.
> > 
> 
> I accept this is not a good naming.
> 
> > But more importantly, this is really a Rust problem. Does Rust really 
> > have no NUL-terminated strings? It should hide them in shame and 
> 
> You can create NUL-terminated strings in Rust of course, but in this
> case, because we want to use the "#[trace_caller]" attribute [1], which
> allows might_sleep() in Rust to be defined as a function, and can use
> Location::caller() to get the caller file and line number information,
> and `Location` type yet doesn't return a Nul-terminated string literal,
> so we have to work this around.
> 
> > construct proper, robust strings, instead of spreading this disease to 
> > the rest of the kernel, IMHO ...
> > 
> > Rust is supposed to be about increased security, right? How does extra, 
> > nonsensical complexity for simple concepts such as strings achieve 
> > that? If the Rust runtime wants to hook into debug facilities of the 
> > Linux kernel then I have bad news: almost all strings used by kernel 
> > debugging facilities are NUL-terminated.
> 
> This is more of a special case because `Location` is used (i.e. file
> name is the string literal). For things like user-defined string, we use
> the macro c_str!(), which generates NUL-terminated strings. For example,
> lockdep class names.
> 
> > 
> > So I really don't like this patch. Is there no other way to do this?
> > 
> 

Trying to see if we can make some forward-progress on this one,
considering:

1. #[track_caller] is really a desired feature to be used for Rust's
   might_sleep(), Alice's reply [3] also explains a bit more on the
   "why" part.

2. To achieve #1, we will need to handle the file name returned by
   Rust's `Location` struct, especially Location::file() will return a
   string literal without a tailing NUL.

3. Other than the current approach proposed by this patch, if the
   existing might_sleep() functionality does not couple (task) state
   inquiries with debug printing, we can maybe avoid printing the
   non-NUL-terminated string in C's __might_sleep*() function by
   printing Location::file() in Rust code:

    #[track_caller]
    fn might_sleep() {
        let loc = Location::caller();

	if (__might_sleep_is_violated()) {
	    pr_err!("BUG: sleeping function called from invalid context at {loc}\n");
	    
	    ...
	}
    }

    but this essentially would add more changes into C code compared to
    the current patch.

4. This is only a special case where we need the "debug information"
   provided by Rust, so this won't happen a lot; and printing a
   non-NUL-terminated string is already supported by printk already, so
   we reuse what kernel already has here.

Given the above, I think the current patch is the best solution.

[3]: https://lore.kernel.org/lkml/aB3I62o8hWSULGBm@google.com/

Regards,
Boqun

> There's a `c_str` [2] macro which could generates a NUL-terminated
> string, but using that will requires might_sleep() defined as a macro as
> well. Given that might_sleep() is the user interface that most users
> will use, and how it handles string literal for file names is an
> implementation detail, so I figured it's better we resolve in the
> current way.
> 
> [1]: https://rustc-dev-guide.rust-lang.org/backend/implicit-caller-location.html
> [2]: https://rust.docs.kernel.org/kernel/macro.c_str.html
> 
> Regards,
> Boqun
> 
> > Thanks,
> > 
> > 	Ingo

On Mon, May 19, 2025 at 05:40:51AM -0700, Boqun Feng wrote:
> On Fri, May 09, 2025 at 12:19:03AM -0700, Boqun Feng wrote:
> > On Fri, May 09, 2025 at 08:00:32AM +0200, Ingo Molnar wrote:
> > > 
> > > * Boqun Feng <boqun.feng@gmail.com> wrote:
> > > 
> > > > From: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > > 
> > > > Add __might_sleep_precision(), Rust friendly version of
> > > > __might_sleep(), which takes a pointer to a string with the length
> > > > instead of a null-terminated string.
> > > > 
> > > > Rust's core::panic::Location::file(), which gives the file name of a
> > > > caller, doesn't provide a null-terminated
> > > > string. __might_sleep_precision() uses a precision specifier in the
> > > > printk format, which specifies the length of a string; a string
> > > > doesn't need to be a null-terminated.
> > > > 
> > > > Modify __might_sleep() to call __might_sleep_precision() but the
> > > > impact should be negligible. When printing the error (sleeping
> > > > function called from invalid context), the precision string format is
> > > > used instead of the simple string format; the precision specifies the
> > > > the maximum length of the displayed string.
> > > > 
> > > > Note that Location::file() providing a null-terminated string for
> > > > better C interoperability is under discussion [1].
> > > > 
> > > > [1]: https://github.com/rust-lang/libs-team/issues/466
> > > > 
> > > > Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > > > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > > Link: https://lore.kernel.org/r/20250410225623.152616-2-fujita.tomonori@gmail.com
> > > > ---
> > > >  include/linux/kernel.h |  2 ++
> > > >  kernel/sched/core.c    | 62 ++++++++++++++++++++++++++++--------------
> > > >  2 files changed, 43 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > > > index be2e8c0a187e..086ee1dc447e 100644
> > > > --- a/include/linux/kernel.h
> > > > +++ b/include/linux/kernel.h
> > > > @@ -87,6 +87,7 @@ extern int dynamic_might_resched(void);
> > > >  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
> > > >  extern void __might_resched(const char *file, int line, unsigned int offsets);
> > > >  extern void __might_sleep(const char *file, int line);
> > > > +extern void __might_sleep_precision(const char *file, int len, int line);
> > > 
> > > Ugh.
> > > 
> > > Firstly, '_precision' is really ambiguous in this context and suggests 
> > > 'precise sleep' or something like that, which this is not about at all. 
> > > So the naming here is all sorts of bad already.
> > > 
> > 
> > I accept this is not a good naming.
> > 
> > > But more importantly, this is really a Rust problem. Does Rust really 
> > > have no NUL-terminated strings? It should hide them in shame and 
> > 
> > You can create NUL-terminated strings in Rust of course, but in this
> > case, because we want to use the "#[trace_caller]" attribute [1], which
> > allows might_sleep() in Rust to be defined as a function, and can use
> > Location::caller() to get the caller file and line number information,
> > and `Location` type yet doesn't return a Nul-terminated string literal,
> > so we have to work this around.
> > 
> > > construct proper, robust strings, instead of spreading this disease to 
> > > the rest of the kernel, IMHO ...
> > > 
> > > Rust is supposed to be about increased security, right? How does extra, 
> > > nonsensical complexity for simple concepts such as strings achieve 
> > > that? If the Rust runtime wants to hook into debug facilities of the 
> > > Linux kernel then I have bad news: almost all strings used by kernel 
> > > debugging facilities are NUL-terminated.
> > 
> > This is more of a special case because `Location` is used (i.e. file
> > name is the string literal). For things like user-defined string, we use
> > the macro c_str!(), which generates NUL-terminated strings. For example,
> > lockdep class names.
> > 
> > > 
> > > So I really don't like this patch. Is there no other way to do this?
> > > 
> > 
> 
> Trying to see if we can make some forward-progress on this one,
> considering:
> 
> 1. #[track_caller] is really a desired feature to be used for Rust's
>    might_sleep(), Alice's reply [3] also explains a bit more on the
>    "why" part.
> 
> 2. To achieve #1, we will need to handle the file name returned by
>    Rust's `Location` struct, especially Location::file() will return a
>    string literal without a tailing NUL.
> 
> 3. Other than the current approach proposed by this patch, if the
>    existing might_sleep() functionality does not couple (task) state
>    inquiries with debug printing, we can maybe avoid printing the
>    non-NUL-terminated string in C's __might_sleep*() function by
>    printing Location::file() in Rust code:
> 
>     #[track_caller]
>     fn might_sleep() {
>         let loc = Location::caller();
> 
> 	if (__might_sleep_is_violated()) {
> 	    pr_err!("BUG: sleeping function called from invalid context at {loc}\n");
> 	    
> 	    ...
> 	}
>     }
> 
>     but this essentially would add more changes into C code compared to
>     the current patch.
> 
> 4. This is only a special case where we need the "debug information"
>    provided by Rust, so this won't happen a lot; and printing a
>    non-NUL-terminated string is already supported by printk already, so
>    we reuse what kernel already has here.
> 
> Given the above, I think the current patch is the best solution.
> 

Ingo,

Alice made some progress on providing the NUL-terminated string for `Location`
[4] [5], which means in the future, we can avoid the __might_sleep_precision()
workaround here, and yet remain the benefit of `#[track_caller]` (Thanks
Alice!). This also means we'd better keep the workaround right now, because that
keeps the same interface if we have NUL-terminated string from
`Location::file()`. And we can revert this workaround easily when the feature is
available in Rust. So I think we should take this.

Moving forwards, let me know if you need me to resend the pull request (there
are also a very trivial improvements in it as well), and I could rename
__might_sleep_precision() to something else (like __might_sleep_nonnulfilename()
or anything) in the resend. Thoughts?

[4]: https://github.com/rust-lang/libs-team/issues/466#issuecomment-2914476468
[5]: https://github.com/rust-lang/rust/issues/141727

Regards,
Boqun

> [3]: https://lore.kernel.org/lkml/aB3I62o8hWSULGBm@google.com/
> 
> Regards,
> Boqun
> 
> > There's a `c_str` [2] macro which could generates a NUL-terminated
> > string, but using that will requires might_sleep() defined as a macro as
> > well. Given that might_sleep() is the user interface that most users
> > will use, and how it handles string literal for file names is an
> > implementation detail, so I figured it's better we resolve in the
> > current way.
> > 
> > [1]: https://rustc-dev-guide.rust-lang.org/backend/implicit-caller-location.html
> > [2]: https://rust.docs.kernel.org/kernel/macro.c_str.html
> > 
> > Regards,
> > Boqun
> > 
> > > Thanks,
> > > 
> > > 	Ingo