Re: [PATCH] liveupdate: truncate getfile name to NAME_MAX

[Pasha Tatashin <pasha.tatashin@soleen.com>]

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