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

[Luca Boccassi <luca.boccassi@gmail.com>]

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.