public inbox for linux-mm@kvack.org
 help / color / mirror / Atom feed
* [PATCH] liveupdate: truncate getfile name to NAME_MAX
@ 2026-03-31 17:54 luca.boccassi
  2026-03-31 18:21 ` Pasha Tatashin
  0 siblings, 1 reply; 7+ messages in thread
From: luca.boccassi @ 2026-03-31 17:54 UTC (permalink / raw)
  To: kexec; +Cc: linux-mm, graf, rppt, pasha.tatashin, pratyush, Luca Boccassi

From: Luca Boccassi <luca.boccassi@gmail.com>

dynamic_dname() harcodes its limit to NAME_MAX bytes, which includes a
NUL terminator and a 'anon_inode:' prefix. If the name passed on goes
above the limit it results in userspace getting a ENAMETOOLONG error.

Truncate the name to NAME_MAX - 12 - 1 characters to ensure this never
fails (accounting for prefix and NUL).

Fixes: 0153094d03df ("liveupdate: luo_session: add sessions support")

Signed-off-by: Luca Boccassi <luca.boccassi@gmail.com>
---
Builds on top of this just sent by Aleksa:

https://lore.kernel.org/linux-fsdevel/20260401-dynamic-dname-name_max-v1-1-8ca20ab2642e@amutable.com/

 kernel/liveupdate/luo_session.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/liveupdate/luo_session.c b/kernel/liveupdate/luo_session.c
index 7836772956406..4a40c7fdfb44f 100644
--- a/kernel/liveupdate/luo_session.c
+++ b/kernel/liveupdate/luo_session.c
@@ -366,11 +366,14 @@ static const struct file_operations luo_session_fops = {
 /* Create a "struct file" for session */
 static int luo_session_getfile(struct luo_session *session, struct file **filep)
 {
-	char name_buf[128];
+	char name_buf[NAME_MAX];
 	struct file *file;
 
 	lockdep_assert_held(&session->mutex);
-	snprintf(name_buf, sizeof(name_buf), "[luo_session] %s", session->name);
+	/* dynamic_dname() rejects names above NAME_MAX bytes, including NUL terminator
+	 * and a 'anon_inode:' prefix. Truncate to NAME_MAX - 12 - 1 to avoid
+	 * ENAMETOOLONG. */
+	snprintf(name_buf, NAME_MAX - 12 - 1, "[luo_session] %s", session->name);
 	file = anon_inode_getfile(name_buf, &luo_session_fops, session, O_RDWR);
 	if (IS_ERR(file))
 		return PTR_ERR(file);
-- 
2.47.3



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] liveupdate: truncate getfile name to NAME_MAX
  2026-03-31 17:54 [PATCH] liveupdate: truncate getfile name to NAME_MAX luca.boccassi
@ 2026-03-31 18:21 ` Pasha Tatashin
  2026-03-31 18:30   ` Luca Boccassi
  0 siblings, 1 reply; 7+ messages in thread
From: Pasha Tatashin @ 2026-03-31 18:21 UTC (permalink / raw)
  To: luca.boccassi; +Cc: kexec, linux-mm, graf, rppt, pratyush

On Tue, Mar 31, 2026 at 1:55 PM <luca.boccassi@gmail.com> wrote:
>
> From: Luca Boccassi <luca.boccassi@gmail.com>
>
> dynamic_dname() harcodes its limit to NAME_MAX bytes, which includes a
> NUL terminator and a 'anon_inode:' prefix. If the name passed on goes
> above the limit it results in userspace getting a ENAMETOOLONG error.
>
> Truncate the name to NAME_MAX - 12 - 1 characters to ensure this never
> fails (accounting for prefix and NUL).
>
> Fixes: 0153094d03df ("liveupdate: luo_session: add sessions support")
>
> Signed-off-by: Luca Boccassi <luca.boccassi@gmail.com>
> ---
> Builds on top of this just sent by Aleksa:
>
> https://lore.kernel.org/linux-fsdevel/20260401-dynamic-dname-name_max-v1-1-8ca20ab2642e@amutable.com/
>
>  kernel/liveupdate/luo_session.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/liveupdate/luo_session.c b/kernel/liveupdate/luo_session.c
> index 7836772956406..4a40c7fdfb44f 100644
> --- a/kernel/liveupdate/luo_session.c
> +++ b/kernel/liveupdate/luo_session.c
> @@ -366,11 +366,14 @@ static const struct file_operations luo_session_fops = {
>  /* Create a "struct file" for session */
>  static int luo_session_getfile(struct luo_session *session, struct file **filep)
>  {
> -       char name_buf[128];
> +       char name_buf[NAME_MAX];
>         struct file *file;
>
>         lockdep_assert_held(&session->mutex);
> -       snprintf(name_buf, sizeof(name_buf), "[luo_session] %s", session->name);
> +       /* dynamic_dname() rejects names above NAME_MAX bytes, including NUL terminator
> +        * and a 'anon_inode:' prefix. Truncate to NAME_MAX - 12 - 1 to avoid
> +        * ENAMETOOLONG. */
> +       snprintf(name_buf, NAME_MAX - 12 - 1, "[luo_session] %s", session->name);

I am confused by this change, the maximum session name length is: 64,
as defined in:
include/uapi/linux/liveupdate.h
/* The maximum length of session name including null termination */
#define LIVEUPDATE_SESSION_NAME_LENGTH 64

So, 128 should be enough to include:
"[luo_session] %s", session->name

Or am I missing something?

Pasha


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] liveupdate: truncate getfile name to NAME_MAX
  2026-03-31 18:21 ` Pasha Tatashin
@ 2026-03-31 18:30   ` Luca Boccassi
  2026-03-31 19:13     ` Pasha Tatashin
  0 siblings, 1 reply; 7+ messages in thread
From: Luca Boccassi @ 2026-03-31 18:30 UTC (permalink / raw)
  To: Pasha Tatashin; +Cc: kexec, linux-mm, Alexander Graf, Mike Rapoport, pratyush

[-- Attachment #1: Type: text/plain, Size: 2617 bytes --]

On Tue, 31 Mar 2026, 19:21 Pasha Tatashin, <pasha.tatashin@soleen.com>
wrote:

> On Tue, Mar 31, 2026 at 1:55 PM <luca.boccassi@gmail.com> wrote:
> >
> > From: Luca Boccassi <luca.boccassi@gmail.com>
> >
> > dynamic_dname() harcodes its limit to NAME_MAX bytes, which includes a
> > NUL terminator and a 'anon_inode:' prefix. If the name passed on goes
> > above the limit it results in userspace getting a ENAMETOOLONG error.
> >
> > Truncate the name to NAME_MAX - 12 - 1 characters to ensure this never
> > fails (accounting for prefix and NUL).
> >
> > Fixes: 0153094d03df ("liveupdate: luo_session: add sessions support")
> >
> > Signed-off-by: Luca Boccassi <luca.boccassi@gmail.com>
> > ---
> > Builds on top of this just sent by Aleksa:
> >
> >
> https://lore.kernel.org/linux-fsdevel/20260401-dynamic-dname-name_max-v1-1-8ca20ab2642e@amutable.com/
> >
> >  kernel/liveupdate/luo_session.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/liveupdate/luo_session.c
> b/kernel/liveupdate/luo_session.c
> > index 7836772956406..4a40c7fdfb44f 100644
> > --- a/kernel/liveupdate/luo_session.c
> > +++ b/kernel/liveupdate/luo_session.c
> > @@ -366,11 +366,14 @@ static const struct file_operations
> luo_session_fops = {
> >  /* Create a "struct file" for session */
> >  static int luo_session_getfile(struct luo_session *session, struct file
> **filep)
> >  {
> > -       char name_buf[128];
> > +       char name_buf[NAME_MAX];
> >         struct file *file;
> >
> >         lockdep_assert_held(&session->mutex);
> > -       snprintf(name_buf, sizeof(name_buf), "[luo_session] %s",
> session->name);
> > +       /* dynamic_dname() rejects names above NAME_MAX bytes, including
> NUL terminator
> > +        * and a 'anon_inode:' prefix. Truncate to NAME_MAX - 12 - 1 to
> avoid
> > +        * ENAMETOOLONG. */
> > +       snprintf(name_buf, NAME_MAX - 12 - 1, "[luo_session] %s",
> session->name);
>
> I am confused by this change, the maximum session name length is: 64,
> as defined in:
> include/uapi/linux/liveupdate.h
> /* The maximum length of session name including null termination */
> #define LIVEUPDATE_SESSION_NAME_LENGTH 64
>
> So, 128 should be enough to include:
> "[luo_session] %s", session->name
>
> Or am I missing something?
>
> Pasha
>

Yeah we need to to bump that size, it's too small - see the shared doc.
This is anyway needed for safety, as the user space call falls apart
completely if it goes over the hard coded limit, so it should always be
used as a max value

>

[-- Attachment #2: Type: text/html, Size: 3879 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] liveupdate: truncate getfile name to NAME_MAX
  2026-03-31 18:30   ` Luca Boccassi
@ 2026-03-31 19:13     ` Pasha Tatashin
  2026-03-31 19:40       ` Luca Boccassi
  0 siblings, 1 reply; 7+ messages in thread
From: Pasha Tatashin @ 2026-03-31 19:13 UTC (permalink / raw)
  To: Luca Boccassi; +Cc: kexec, linux-mm, Alexander Graf, Mike Rapoport, pratyush

On Tue, Mar 31, 2026 at 2:30 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
>
> On Tue, 31 Mar 2026, 19:21 Pasha Tatashin, <pasha.tatashin@soleen.com> wrote:
>>
>> On Tue, Mar 31, 2026 at 1:55 PM <luca.boccassi@gmail.com> wrote:
>> >
>> > From: Luca Boccassi <luca.boccassi@gmail.com>
>> >
>> > dynamic_dname() harcodes its limit to NAME_MAX bytes, which includes a
>> > NUL terminator and a 'anon_inode:' prefix. If the name passed on goes
>> > above the limit it results in userspace getting a ENAMETOOLONG error.
>> >
>> > Truncate the name to NAME_MAX - 12 - 1 characters to ensure this never
>> > fails (accounting for prefix and NUL).
>> >
>> > Fixes: 0153094d03df ("liveupdate: luo_session: add sessions support")

Let's remove this, this patch is not fixing an existing issue, but a
preparation for future uapi changes?

>> >
>> > Signed-off-by: Luca Boccassi <luca.boccassi@gmail.com>
>> > ---
>> > Builds on top of this just sent by Aleksa:
>> >
>> > https://lore.kernel.org/linux-fsdevel/20260401-dynamic-dname-name_max-v1-1-8ca20ab2642e@amutable.com/
>> >
>> >  kernel/liveupdate/luo_session.c | 7 +++++--
>> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/kernel/liveupdate/luo_session.c b/kernel/liveupdate/luo_session.c
>> > index 7836772956406..4a40c7fdfb44f 100644
>> > --- a/kernel/liveupdate/luo_session.c
>> > +++ b/kernel/liveupdate/luo_session.c
>> > @@ -366,11 +366,14 @@ static const struct file_operations luo_session_fops = {
>> >  /* Create a "struct file" for session */
>> >  static int luo_session_getfile(struct luo_session *session, struct file **filep)
>> >  {
>> > -       char name_buf[128];
>> > +       char name_buf[NAME_MAX];
>> >         struct file *file;
>> >
>> >         lockdep_assert_held(&session->mutex);
>> > -       snprintf(name_buf, sizeof(name_buf), "[luo_session] %s", session->name);
>> > +       /* dynamic_dname() rejects names above NAME_MAX bytes, including NUL terminator
>> > +        * and a 'anon_inode:' prefix. Truncate to NAME_MAX - 12 - 1 to avoid
>> > +        * ENAMETOOLONG. */
>> > +       snprintf(name_buf, NAME_MAX - 12 - 1, "[luo_session] %s", session->name);
>>
>> I am confused by this change, the maximum session name length is: 64,
>> as defined in:
>> include/uapi/linux/liveupdate.h
>> /* The maximum length of session name including null termination */
>> #define LIVEUPDATE_SESSION_NAME_LENGTH 64
>>
>> So, 128 should be enough to include:
>> "[luo_session] %s", session->name
>>
>> Or am I missing something?
>>
>> Pasha
>
>
> Yeah we need to to bump that size, it's too small - see the shared doc. This is anyway needed for safety, as the user space call falls apart completely if it goes over the hard coded limit,

If there is a safety issue, it should be fixed (even without a limit
bump), could you please explain it.

I looked at the shared doc [https://tinyurl.com/luoddesign under limits tab]:

I see:
TODO: this needs to be bumped, as for the varlink API we will impose a
cgroup path prefix, which means PATH_MAX + "/" + client-supplied
string.

Why would we do that? Why can't we use a 63-byte unique identifier for
sessions and an in-memory database that maps the cgroup path prefix to
session_ids, where the database also gets passed from one kernel to
the other?

Increasing this limit would require a kernel uAPI change, which
requires a very strong rationale, and would bump up the KHO ABI for
LUO. It also means that the LUO session data in:

struct luo_session_ser {
   char name[LIVEUPDATE_SESSION_NAME_LENGTH];
   struct luo_file_set_ser file_set_ser;
} __packed;

would get bigger, and thus the kernel would need to preserve more data
for every single session.

Ideally, this extra data should be preserved by the userspace
orchestrator via a separate memfd.

Pasha


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] liveupdate: truncate getfile name to NAME_MAX
  2026-03-31 19:13     ` Pasha Tatashin
@ 2026-03-31 19:40       ` Luca Boccassi
  2026-03-31 19:57         ` Pasha Tatashin
  0 siblings, 1 reply; 7+ messages in thread
From: Luca Boccassi @ 2026-03-31 19:40 UTC (permalink / raw)
  To: Pasha Tatashin; +Cc: kexec, linux-mm, Alexander Graf, Mike Rapoport, pratyush

On Tue, 31 Mar 2026 at 20:14, Pasha Tatashin <pasha.tatashin@soleen.com> wrote:
>
> On Tue, Mar 31, 2026 at 2:30 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
> >
> > On Tue, 31 Mar 2026, 19:21 Pasha Tatashin, <pasha.tatashin@soleen.com> wrote:
> >>
> >> On Tue, Mar 31, 2026 at 1:55 PM <luca.boccassi@gmail.com> wrote:
> >> >
> >> > From: Luca Boccassi <luca.boccassi@gmail.com>
> >> >
> >> > dynamic_dname() harcodes its limit to NAME_MAX bytes, which includes a
> >> > NUL terminator and a 'anon_inode:' prefix. If the name passed on goes
> >> > above the limit it results in userspace getting a ENAMETOOLONG error.
> >> >
> >> > Truncate the name to NAME_MAX - 12 - 1 characters to ensure this never
> >> > fails (accounting for prefix and NUL).
> >> >
> >> > Fixes: 0153094d03df ("liveupdate: luo_session: add sessions support")
>
> Let's remove this, this patch is not fixing an existing issue, but a
> preparation for future uapi changes?

The linked patch that increases the buffer to 255 is tagged with
'fixes' so I don't mind, as that's enough to fix the -ENAMETOOLONG
error in currently released kernels. The original version of this set
the truncation to the current max buffer size, hence why I had the
fixes line.

> >> >
> >> > Signed-off-by: Luca Boccassi <luca.boccassi@gmail.com>
> >> > ---
> >> > Builds on top of this just sent by Aleksa:
> >> >
> >> > https://lore.kernel.org/linux-fsdevel/20260401-dynamic-dname-name_max-v1-1-8ca20ab2642e@amutable.com/
> >> >
> >> >  kernel/liveupdate/luo_session.c | 7 +++++--
> >> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/kernel/liveupdate/luo_session.c b/kernel/liveupdate/luo_session.c
> >> > index 7836772956406..4a40c7fdfb44f 100644
> >> > --- a/kernel/liveupdate/luo_session.c
> >> > +++ b/kernel/liveupdate/luo_session.c
> >> > @@ -366,11 +366,14 @@ static const struct file_operations luo_session_fops = {
> >> >  /* Create a "struct file" for session */
> >> >  static int luo_session_getfile(struct luo_session *session, struct file **filep)
> >> >  {
> >> > -       char name_buf[128];
> >> > +       char name_buf[NAME_MAX];
> >> >         struct file *file;
> >> >
> >> >         lockdep_assert_held(&session->mutex);
> >> > -       snprintf(name_buf, sizeof(name_buf), "[luo_session] %s", session->name);
> >> > +       /* dynamic_dname() rejects names above NAME_MAX bytes, including NUL terminator
> >> > +        * and a 'anon_inode:' prefix. Truncate to NAME_MAX - 12 - 1 to avoid
> >> > +        * ENAMETOOLONG. */
> >> > +       snprintf(name_buf, NAME_MAX - 12 - 1, "[luo_session] %s", session->name);
> >>
> >> I am confused by this change, the maximum session name length is: 64,
> >> as defined in:
> >> include/uapi/linux/liveupdate.h
> >> /* The maximum length of session name including null termination */
> >> #define LIVEUPDATE_SESSION_NAME_LENGTH 64
> >>
> >> So, 128 should be enough to include:
> >> "[luo_session] %s", session->name
> >>
> >> Or am I missing something?
> >>
> >> Pasha
> >
> >
> > Yeah we need to to bump that size, it's too small - see the shared doc. This is anyway needed for safety, as the user space call falls apart completely if it goes over the hard coded limit,
>
> If there is a safety issue, it should be fixed (even without a limit
> bump), could you please explain it.

Nothing specific, more from the abstract point of view - given that
this will fail with a hard error if it goes above the hardcoded limit,
it seems better to me to check it, rather than assuming it's going to
match, as that's error prone.

> I looked at the shared doc [https://tinyurl.com/luoddesign under limits tab]:
>
> I see:
> TODO: this needs to be bumped, as for the varlink API we will impose a
> cgroup path prefix, which means PATH_MAX + "/" + client-supplied
> string.
>
> Why would we do that? Why can't we use a 63-byte unique identifier for
> sessions and an in-memory database that maps the cgroup path prefix to
> session_ids, where the database also gets passed from one kernel to
> the other?

Because that requires tracking state across all runtime, and that's a
no-go. The session itself needs to be identifiable by cgroup, so that
its owner is uniquely identified in all cases, in all situations,
regardless of any state or lack thereof.
The structs have a size as input, so it should be doable to follow the
usual process and keep it backward compatible.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] liveupdate: truncate getfile name to NAME_MAX
  2026-03-31 19:40       ` Luca Boccassi
@ 2026-03-31 19:57         ` Pasha Tatashin
  2026-03-31 20:09           ` Luca Boccassi
  0 siblings, 1 reply; 7+ messages in thread
From: Pasha Tatashin @ 2026-03-31 19:57 UTC (permalink / raw)
  To: Luca Boccassi; +Cc: kexec, linux-mm, Alexander Graf, Mike Rapoport, pratyush

On Tue, Mar 31, 2026 at 3:40 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
>
> On Tue, 31 Mar 2026 at 20:14, Pasha Tatashin <pasha.tatashin@soleen.com> wrote:
> >
> > On Tue, Mar 31, 2026 at 2:30 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
> > >
> > > On Tue, 31 Mar 2026, 19:21 Pasha Tatashin, <pasha.tatashin@soleen.com> wrote:
> > >>
> > >> On Tue, Mar 31, 2026 at 1:55 PM <luca.boccassi@gmail.com> wrote:
> > >> >
> > >> > From: Luca Boccassi <luca.boccassi@gmail.com>
> > >> >
> > >> > dynamic_dname() harcodes its limit to NAME_MAX bytes, which includes a
> > >> > NUL terminator and a 'anon_inode:' prefix. If the name passed on goes
> > >> > above the limit it results in userspace getting a ENAMETOOLONG error.
> > >> >
> > >> > Truncate the name to NAME_MAX - 12 - 1 characters to ensure this never
> > >> > fails (accounting for prefix and NUL).
> > >> >
> > >> > Fixes: 0153094d03df ("liveupdate: luo_session: add sessions support")
> >
> > Let's remove this, this patch is not fixing an existing issue, but a
> > preparation for future uapi changes?
>
> The linked patch that increases the buffer to 255 is tagged with
> 'fixes' so I don't mind, as that's enough to fix the -ENAMETOOLONG
> error in currently released kernels. The original version of this set
> the truncation to the current max buffer size, hence why I had the
> fixes line.
>
> > >> >
> > >> > Signed-off-by: Luca Boccassi <luca.boccassi@gmail.com>
> > >> > ---
> > >> > Builds on top of this just sent by Aleksa:
> > >> >
> > >> > https://lore.kernel.org/linux-fsdevel/20260401-dynamic-dname-name_max-v1-1-8ca20ab2642e@amutable.com/
> > >> >
> > >> >  kernel/liveupdate/luo_session.c | 7 +++++--
> > >> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >> >
> > >> > diff --git a/kernel/liveupdate/luo_session.c b/kernel/liveupdate/luo_session.c
> > >> > index 7836772956406..4a40c7fdfb44f 100644
> > >> > --- a/kernel/liveupdate/luo_session.c
> > >> > +++ b/kernel/liveupdate/luo_session.c
> > >> > @@ -366,11 +366,14 @@ static const struct file_operations luo_session_fops = {
> > >> >  /* Create a "struct file" for session */
> > >> >  static int luo_session_getfile(struct luo_session *session, struct file **filep)
> > >> >  {
> > >> > -       char name_buf[128];
> > >> > +       char name_buf[NAME_MAX];
> > >> >         struct file *file;
> > >> >
> > >> >         lockdep_assert_held(&session->mutex);
> > >> > -       snprintf(name_buf, sizeof(name_buf), "[luo_session] %s", session->name);
> > >> > +       /* dynamic_dname() rejects names above NAME_MAX bytes, including NUL terminator
> > >> > +        * and a 'anon_inode:' prefix. Truncate to NAME_MAX - 12 - 1 to avoid
> > >> > +        * ENAMETOOLONG. */
> > >> > +       snprintf(name_buf, NAME_MAX - 12 - 1, "[luo_session] %s", session->name);
> > >>
> > >> I am confused by this change, the maximum session name length is: 64,
> > >> as defined in:
> > >> include/uapi/linux/liveupdate.h
> > >> /* The maximum length of session name including null termination */
> > >> #define LIVEUPDATE_SESSION_NAME_LENGTH 64
> > >>
> > >> So, 128 should be enough to include:
> > >> "[luo_session] %s", session->name
> > >>
> > >> Or am I missing something?
> > >>
> > >> Pasha
> > >
> > >
> > > Yeah we need to to bump that size, it's too small - see the shared doc. This is anyway needed for safety, as the user space call falls apart completely if it goes over the hard coded limit,
> >
> > If there is a safety issue, it should be fixed (even without a limit
> > bump), could you please explain it.
>
> Nothing specific, more from the abstract point of view - given that
> this will fail with a hard error if it goes above the hardcoded limit,
> it seems better to me to check it, rather than assuming it's going to
> match, as that's error prone.
>
> > I looked at the shared doc [https://tinyurl.com/luoddesign under limits tab]:
> >
> > I see:
> > TODO: this needs to be bumped, as for the varlink API we will impose a
> > cgroup path prefix, which means PATH_MAX + "/" + client-supplied
> > string.
> >
> > Why would we do that? Why can't we use a 63-byte unique identifier for
> > sessions and an in-memory database that maps the cgroup path prefix to
> > session_ids, where the database also gets passed from one kernel to
> > the other?
>
> Because that requires tracking state across all runtime, and that's a
> no-go. The session itself needs to be identifiable by cgroup, so that
> its owner is uniquely identified in all cases, in all situations,
> regardless of any state or lack thereof.
> The structs have a size as input, so it should be doable to follow the
> usual process and keep it backward compatible.

Overall, I am super happy to see LUO integrated as a first-class
citizen with systemd, and I fully support this effort. My suggestion
is that we step back and prepare a single patch series that performs
all the necessary changes together, instead of sending a few random
patches that do not make sense by themselves.

As far as I can see, we should:
1. Increase the session name limit to include the data necessary for
systemd fd store restore capabilities.
2. Make the number of stored sessions dynamic rather than having a
hardcoded limit (I have a patch for this, which we can include in the
series).
3. Add the necessary API to query back the incoming and outgoing session names.

Is there anything else that you are aware that we would need to add?

Thank you,
Pasha


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] liveupdate: truncate getfile name to NAME_MAX
  2026-03-31 19:57         ` Pasha Tatashin
@ 2026-03-31 20:09           ` Luca Boccassi
  0 siblings, 0 replies; 7+ messages in thread
From: Luca Boccassi @ 2026-03-31 20:09 UTC (permalink / raw)
  To: Pasha Tatashin; +Cc: kexec, linux-mm, Alexander Graf, Mike Rapoport, pratyush

On Tue, 31 Mar 2026 at 20:58, Pasha Tatashin <pasha.tatashin@soleen.com> wrote:
>
> On Tue, Mar 31, 2026 at 3:40 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
> >
> > On Tue, 31 Mar 2026 at 20:14, Pasha Tatashin <pasha.tatashin@soleen.com> wrote:
> > >
> > > On Tue, Mar 31, 2026 at 2:30 PM Luca Boccassi <luca.boccassi@gmail.com> wrote:
> > > >
> > > > On Tue, 31 Mar 2026, 19:21 Pasha Tatashin, <pasha.tatashin@soleen.com> wrote:
> > > >>
> > > >> On Tue, Mar 31, 2026 at 1:55 PM <luca.boccassi@gmail.com> wrote:
> > > >> >
> > > >> > From: Luca Boccassi <luca.boccassi@gmail.com>
> > > >> >
> > > >> > dynamic_dname() harcodes its limit to NAME_MAX bytes, which includes a
> > > >> > NUL terminator and a 'anon_inode:' prefix. If the name passed on goes
> > > >> > above the limit it results in userspace getting a ENAMETOOLONG error.
> > > >> >
> > > >> > Truncate the name to NAME_MAX - 12 - 1 characters to ensure this never
> > > >> > fails (accounting for prefix and NUL).
> > > >> >
> > > >> > Fixes: 0153094d03df ("liveupdate: luo_session: add sessions support")
> > >
> > > Let's remove this, this patch is not fixing an existing issue, but a
> > > preparation for future uapi changes?
> >
> > The linked patch that increases the buffer to 255 is tagged with
> > 'fixes' so I don't mind, as that's enough to fix the -ENAMETOOLONG
> > error in currently released kernels. The original version of this set
> > the truncation to the current max buffer size, hence why I had the
> > fixes line.
> >
> > > >> >
> > > >> > Signed-off-by: Luca Boccassi <luca.boccassi@gmail.com>
> > > >> > ---
> > > >> > Builds on top of this just sent by Aleksa:
> > > >> >
> > > >> > https://lore.kernel.org/linux-fsdevel/20260401-dynamic-dname-name_max-v1-1-8ca20ab2642e@amutable.com/
> > > >> >
> > > >> >  kernel/liveupdate/luo_session.c | 7 +++++--
> > > >> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > >> >
> > > >> > diff --git a/kernel/liveupdate/luo_session.c b/kernel/liveupdate/luo_session.c
> > > >> > index 7836772956406..4a40c7fdfb44f 100644
> > > >> > --- a/kernel/liveupdate/luo_session.c
> > > >> > +++ b/kernel/liveupdate/luo_session.c
> > > >> > @@ -366,11 +366,14 @@ static const struct file_operations luo_session_fops = {
> > > >> >  /* Create a "struct file" for session */
> > > >> >  static int luo_session_getfile(struct luo_session *session, struct file **filep)
> > > >> >  {
> > > >> > -       char name_buf[128];
> > > >> > +       char name_buf[NAME_MAX];
> > > >> >         struct file *file;
> > > >> >
> > > >> >         lockdep_assert_held(&session->mutex);
> > > >> > -       snprintf(name_buf, sizeof(name_buf), "[luo_session] %s", session->name);
> > > >> > +       /* dynamic_dname() rejects names above NAME_MAX bytes, including NUL terminator
> > > >> > +        * and a 'anon_inode:' prefix. Truncate to NAME_MAX - 12 - 1 to avoid
> > > >> > +        * ENAMETOOLONG. */
> > > >> > +       snprintf(name_buf, NAME_MAX - 12 - 1, "[luo_session] %s", session->name);
> > > >>
> > > >> I am confused by this change, the maximum session name length is: 64,
> > > >> as defined in:
> > > >> include/uapi/linux/liveupdate.h
> > > >> /* The maximum length of session name including null termination */
> > > >> #define LIVEUPDATE_SESSION_NAME_LENGTH 64
> > > >>
> > > >> So, 128 should be enough to include:
> > > >> "[luo_session] %s", session->name
> > > >>
> > > >> Or am I missing something?
> > > >>
> > > >> Pasha
> > > >
> > > >
> > > > Yeah we need to to bump that size, it's too small - see the shared doc. This is anyway needed for safety, as the user space call falls apart completely if it goes over the hard coded limit,
> > >
> > > If there is a safety issue, it should be fixed (even without a limit
> > > bump), could you please explain it.
> >
> > Nothing specific, more from the abstract point of view - given that
> > this will fail with a hard error if it goes above the hardcoded limit,
> > it seems better to me to check it, rather than assuming it's going to
> > match, as that's error prone.
> >
> > > I looked at the shared doc [https://tinyurl.com/luoddesign under limits tab]:
> > >
> > > I see:
> > > TODO: this needs to be bumped, as for the varlink API we will impose a
> > > cgroup path prefix, which means PATH_MAX + "/" + client-supplied
> > > string.
> > >
> > > Why would we do that? Why can't we use a 63-byte unique identifier for
> > > sessions and an in-memory database that maps the cgroup path prefix to
> > > session_ids, where the database also gets passed from one kernel to
> > > the other?
> >
> > Because that requires tracking state across all runtime, and that's a
> > no-go. The session itself needs to be identifiable by cgroup, so that
> > its owner is uniquely identified in all cases, in all situations,
> > regardless of any state or lack thereof.
> > The structs have a size as input, so it should be doable to follow the
> > usual process and keep it backward compatible.
>
> Overall, I am super happy to see LUO integrated as a first-class
> citizen with systemd, and I fully support this effort. My suggestion
> is that we step back and prepare a single patch series that performs
> all the necessary changes together, instead of sending a few random
> patches that do not make sense by themselves.

Yeah I don't mind, I noticed these two issues and fixed them as I was
going, so just thought of sending them as I verified they work.
Thankfully Aleksa's patch is enough to fix the ENAMETOOLONG issue and
should get into the released kernels too, so this can wait.

> As far as I can see, we should:
> 1. Increase the session name limit to include the data necessary for
> systemd fd store restore capabilities.
> 2. Make the number of stored sessions dynamic rather than having a
> hardcoded limit (I have a patch for this, which we can include in the
> series).
> 3. Add the necessary API to query back the incoming and outgoing session names.
>
> Is there anything else that you are aware that we would need to add?

Yeah also mentioned in the doc - we should bump the limit of FDs per
session, as for the fdstore I am using a single session for the whole
system, as we talked about two weeks ago. Apart from this I haven't
come across anything, but I just got the initial proof of concept to
work end to end this morning so haven't had a lot of time. Also the
per-client session is still in progress. If I find more requirements
I'll definitely bring them up ASAP.


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-03-31 20:09 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-31 17:54 [PATCH] liveupdate: truncate getfile name to NAME_MAX luca.boccassi
2026-03-31 18:21 ` Pasha Tatashin
2026-03-31 18:30   ` Luca Boccassi
2026-03-31 19:13     ` Pasha Tatashin
2026-03-31 19:40       ` Luca Boccassi
2026-03-31 19:57         ` Pasha Tatashin
2026-03-31 20:09           ` Luca Boccassi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox