AI generated content:

Found while reviewing the kernel-doc conversion of
lustre/obdclass/jobid.c (change 66941, commit c31894b79857). A
documentation-only follow-up to that change documents the behaviour
described below as it stands today; this ticket is for actually fixing
it. No in-tree caller checks the return value, so none of this is
visible as a failure today. Every claim below was traced to a specific
line in current master.

== Problem 1: "0 on success" is not what it returns ==

The kernel-doc added by c31894b79857 said:

Return: %0 on success, %-errno on error

On the per-session / process-environment path the value from
jobid_get_from_cache() is passed straight out of lustre_get_jobid(),
and that helper ends with:

lustre/obdclass/jobid.c, jobid_get_from_cache()
out:
return rc < 0 ? rc : joblen;

where joblen has been reassigned to a jobid length. So a direct lookup
hit returns a positive number, not 0. The other paths - JOBSTATS_NODELOCAL,
JOBSTATS_PROCNAME_UID, and any fallback through jobid_interpret_string()

• return 0. The function is therefore inconsistent with itself as well
  as with its doc.

Worse, the positive value is not reliably a length of anything (see
Problem 3).

Both in-tree callers ignore the value:

lustre/llite/vvp_io.c:1847 (in vvp_io_init())
lustre/ptlrpc/pack_generic.c (in lustre_msg_set_jobinfo())

but lustre_get_jobid() is EXPORT_SYMBOL'd, and a return that is
sometimes 0 and sometimes a length is a trap for the next caller that
writes the obvious "if (rc) goto err".

== Problem 2: success with an undefined output buffer ==

There are at least two ways lustre_get_jobid() returns 0 without
defining @jobid. The second is the broader one and does not need a
kernel thread.

(a) No branch runs at all. lustre_get_jobid() has no terminal else:

if (obd_jobid_var == JOBSTATS_DISABLE)

{ memset; RETURN(0); }

if (obd_jobid_var == JOBSTATS_NODELOCAL) rc = ...;
else if (obd_jobid_var == JOBSTATS_PROCNAME_UID) rc = ...;
else if (obd_jobid_var == JOBSTATS_SESSION ||
jobid_name_is_valid(current->comm))

{ ... }

RETURN(rc); /* still the initial 0 if nothing matched */

So when obd_jobid_var names an environment variable (the usual HPC
configuration, e.g. "SLURM_JOB_ID") and jobid_name_is_valid(current->comm)
is false, rc stays 0 and the buffer is never touched.
jobid_name_is_valid() rejects exactly the Lustre and kernel service
thread names - "kworker", "kswapd", "writeback", "irq", "ksoftirq",
"ptlrpc", "ldlm", "ll_ping", "ll_sa", "ll_ucp".

(b) obd_jobid_name is "%j" and the lookup fails. This one fires in
ordinary user context. When obd_jobid_name contains "%j" the direct
jobid_get_from_cache() call is skipped, and the fallback expands
obd_jobid_name through jobid_interpret_string(), whose "%j" case is:

case 'j': /* jobid stored in process environment */
l = jobid_get_from_cache(jobid, width);
if (l < 0)
l = 0;

On failure l is 0, so nothing is written and neither jobid nor joblen
advances. The function then falls out to:

out:
jobid[joblen - 1] = '\0';
return joblen < 0 ? -EOVERFLOW : 0;

i.e. it clears only the last byte of the capped buffer and returns
0. jobid[0 .. joblen-2] is whatever the caller's buffer held.
lustre_get_jobid() maps that rc2 == 0 to rc = 0 and returns success.

== Where the undefined buffer goes ==

lustre/llite/vvp_io.c, vvp_io_init()
struct job_info ji; /* uninitialised */
...
lustre_get_jobid(ji.ji_jobid, sizeof(ji.ji_jobid)); /* rc ignored */
ji.ji_uid = ...; ji.ji_gid = ...;
write_seqlock(&lli->lli_jobinfo_seqlock);
memcpy(&lli->lli_jobinfo, &ji, sizeof(ji)); /* unconditional */
write_sequnlock(&lli->lli_jobinfo_seqlock);

lli_jobinfo is later read by osc to fill pb_jobid on outgoing RPCs, so
this is kernel stack residue reaching the wire and server-side jobstats.

Two mitigations already exist, and both are accidental rather than
designed:

• lustre/llite/rw.c, in the async readahead path, does right after
  cl_io_rw_init() has run vvp_io_init():

/* overwrite jobid inited in vvp_io_init() */
write_seqlock(&lli->lli_jobinfo_seqlock);
memcpy(&lli->lli_jobinfo, &work->lrw_jobinfo, sizeof(...));
write_sequnlock(&lli->lli_jobinfo_seqlock);

That comment is evidence the problem is known at that one site. It
is still a fixup after the fact - lli_jobinfo holds the garbage for
the remainder of cl_io_rw_init(), where a concurrent reader on the
same inode can pick it up - and it does nothing for any other entry
into vvp_io_init().

• lustre_msg_set_jobinfo() is guarded by pb_jobid[0] == '\0' on an
  already-zeroed message buffer, so a no-write leaves it empty rather
  than filled with garbage. That is luck, not a contract.

The default obd_jobid_var is "disable", which memsets and returns early,
so a stock configuration is not affected.

== Problem 3: the returned length is not a length ==

Two independent defects mean the positive value from
jobid_get_from_cache() can exceed both strlen(@jobid) and the caller's
@joblen:

• cfs_get_environ() does not update *val_len when it truncates:

if (entry_len >= *val_len)

{ memcpy(value, entry, *val_len); value[*val_len - 1] = 0; GOTO(out, rc = -EOVERFLOW); }

memcpy(value, entry, entry_len);
val_len = entry_len; / only on the success path */

and jobid_get_from_environ() deliberately converts that -EOVERFLOW
to rc = 0 (with a rate-limited LCONSOLE_WARN). So the caller takes
the success branch with *val_len still equal to the buffer size.

• jobid_get_from_cache() then stores that as the cached length and
  later returns it verbatim:

pidmap->jp_joblen = env_len;
strscpy(pidmap->jp_jobid, env_jobid, sizeof(pidmap->jp_jobid));
...
strscpy(jobid, pidmap->jp_jobid, joblen);
joblen = pidmap->jp_joblen;

Both strscpy() calls can truncate, and neither result feeds
jp_joblen.

== Suggested fix ==

1. Make lustre_get_jobid() always define the output buffer - an
unconditional jobid[0] = '\0' up front, or a terminal else - so a 0
return always means "@jobid is set". This also covers the "%j"
fallback path in jobid_interpret_string().
2. Normalise lustre_get_jobid() to return 0-or-negative. Neither
in-tree caller wants a length; the simplest form is to stop
propagating jobid_get_from_cache()'s length out. Whether that
helper should keep returning a length at all is a separate call -
given Problem 3, returning it is arguably worse than useless.
3. Zero-initialise "struct job_info ji" in vvp_io_init() regardless,
since it is memcpy'd wholesale into the inode.
4. Fix the truncation accounting: have cfs_get_environ() set *val_len
to what it actually copied on the -EOVERFLOW path, and have
jobid_get_from_cache() derive jp_joblen from the strscpy() result
rather than from env_len.
5. With 1-4 in place, re-examine the rw.c "overwrite jobid inited in
vvp_io_init()" fixup. It may still be wanted so readahead records
the submitting job rather than the kworker, but it should no longer
be load-bearing for correctness.
6. Update the kernel-doc Return: section to match, replacing whatever
the doc-only follow-up to 66941 lands with.

== Reproducer sketch (untested) ==

1. Problem 2(b), ordinary user context:
   lctl set_param jobid_var=SLURM_JOB_ID jobid_name='%j'
2. run I/O from a process with no SLURM_JOB_ID in its environment
3. instrument vvp_io_init() to dump ji.ji_jobid before the memcpy

1. Problem 2(a):
   lctl set_param jobid_var=SLURM_JOB_ID
2. drive async readahead so ll_readahead_handle_work() runs on a kworker
3. (that path is patched over in rw.c; instrument vvp_io_init() itself)