Re: [PATCH v7 for-next 3/8] livepatch: Implement replace set for scoped atomic replace

[Josh Poimboeuf <jpoimboe@kernel.org>]

On Fri, Aug 28, 2026 at 11:03:02AM +0800, Yafang Shao wrote:
> On Fri, Aug 28, 2026 at 8:26 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > +     # Remove duplicates from obsoletes (if specified)
> > > +     # Note: provides ID is not added here; the kernel will replace
> > > +     # livepatch with the same provides ID.
> > > +     if [[ -n "$OBSOLETES" ]]; then
> > > +             local obsoletes_clean="${OBSOLETES//[ ]/}"
> > > +             local IFS=','
> > > +             local -a obs_array=()
> > > +             local obs_id
> > > +             local already_exists
> > > +
> > > +             for obs_id in $obsoletes_clean; do
> > > +                     if [[ -n "$obs_id" ]]; then
> > > +                             already_exists=0
> > > +                             for existing in "${obs_array[@]}"; do
> > > +                                     if [[ "$existing" -eq "$obs_id" ]]; then
> > > +                                             already_exists=1
> > > +                                             break
> > > +                                     fi
> > > +                             done
> > > +
> > > +                             if [[ "$already_exists" -eq 0 ]]; then
> > > +                                     obs_array+=("$obs_id")
> > > +                             fi
> > > +                     fi
> > > +             done
> > > +
> > > +             local IFS=','
> > > +             OBSOLETES="${obs_array[*]}"
> > > +             unset IFS
> > > +     fi
> > > +
> >
> > What's the point of this?   Do we expect duplicates,
> 
> Duplicated ids are indeed a user input error (e.g. -r "1,1,2,3") and
> should not be expected.
> 
> > and if so, wouldn't
> > it be the kernel's job to handle that?
> 
> On the kernel side the duplicates are safe:
> klp_patch_replaces() iterates over the obsoletes array and compares
> each entry against the other patches' provides, so a duplicated id is
> simply idempotent - the only side effect is duplicated ids shown in
> the /sys/kernel/livepatch/<patch>/obsoletes attribute.
> 
> Silently deduplicating in klp-build was masking the input mistake.
> Agreed on the fail-fast approach: klp-build will now error out with a
> clear message when duplicate obsoletes ids are detected.  The list
> passed to the kernel is then guaranteed clean, and the sysfs side
> effect cannot happen.

There are other ways of building livepatches beyond klp-build, and this
wouldn't prevent those other patch generation methods from making the
same mistake.

But since it doesn't actually break anything, and the user is not likely
to be setting multiple obsoletes, much less introducing duplicates, I
would say just remove the check altogether.  It's not really a bug and I
doubt that somebody who is careless enough to have duplicate obsoletes
would notice or care that sysfs also has duplicates... and if they do
care, they should fix their patch :-)

For similar reasons I think some of the other error checking isn't
really needed (invalid comma usage, negative number check) as those will
either fail the build or will give the user what they asked for, not
dissimilar from providing a buggy patch.

Then I think the majority of those klp-build changes aren't needed, and
it can be a simple passthrough of the user cmdline:

  cflags+=("-DKLP_OBSOLETES=$OBSOLETES")

-- 
Josh