Re: [RFC PATCH v4 3/4] livepatch: Implement replace set for scoped atomic replace

On Tue, Aug 4, 2026 at 3:03 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Dynamically allocated `funcs` arrays are leaked on KLP_OBSOLETES parsing failure.
> --
>
> --- Patch [3]: [RFC PATCH v4 3/4] livepatch: Implement replace set for scoped atomic replace ---
> commit d186609c1a3dbe210857d0040f657fa918504faf
> Author: Yafang Shao <[email protected]>
> Subject: livepatch: Implement replace set for scoped atomic replace
>
> This commit introduces a more flexible atomic replace model for livepatches
> using provides and obsoletes fields. A new patch will replace any existing
> patch with a matching provides ID or any ID listed in its obsoletes array.
> The klp-build script and init code are updated to parse and validate these
> new fields.
>
> > diff --git a/scripts/livepatch/init.c b/scripts/livepatch/init.c
> > --- a/scripts/livepatch/init.c
> > +++ b/scripts/livepatch/init.c
> [ ... ]
> > @@ -72,10 +73,73 @@ static int __init livepatch_mod_init(void)
> [ ... ]
> > +#ifdef KLP_OBSOLETES
> > +     /*
> > +      * Parse KLP_OBSOLETES string (format: "0,1,2" or "0" or empty)
> > +      * and convert to unsigned int array for patch->obsoletes
> > +      *
> > +      * Note: KLP_OBSOLETES is always defined and includes at least the
> > +      * provides ID, ensuring the patch replaces old patches with same ID.
> > +      */
> > +     {
> > +             unsigned int *obs_array;
> > +             unsigned int count = 1;
> > +             char *obsoletes_str;
> > +             char *token, *str;
> > +             int i = 0;
> > +
> > +             for (str = (char *)KLP_OBSOLETES; *str; str++) {
> > +                     if (*str == ',')
> > +                             count++;
> > +             }
> > +
> > +             obsoletes_str = kstrdup(KLP_OBSOLETES, GFP_KERNEL);
> > +             if (!obsoletes_str) {
> > +                     ret = -ENOMEM;
> > +                     goto err_free_objs;
> > +             }
> > +
> > +             obs_array = kmalloc_array(count, sizeof(unsigned int), GFP_KERNEL);
> > +             if (!obs_array) {
> > +                     kfree(obsoletes_str);
> > +                     ret = -ENOMEM;
> > +                     goto err_free_objs;
> > +             }
> > +
> > +             str = obsoletes_str;
> > +             while ((token = strsep(&str, ",")) != NULL) {
> > +                     unsigned int val;
> > +
> > +                     ret = kstrtouint(token, 10, &val);
> > +                     if (ret) {
> > +                             kfree(obsoletes_str);
> > +                             kfree(obs_array);
> > +                             goto err_free_objs;
> > +                     }
> > +                     obs_array[i++] = val;
> > +             }
>
> [Severity: Medium]
> Does this code leak the funcs arrays on error?
>
> Earlier in livepatch_mod_init(), the funcs arrays are dynamically allocated
> for each object in a loop:
>
>         funcs = kzalloc(sizeof(struct klp_func) * (nr_funcs + 1), GFP_KERNEL);
>         ...
>         obj->funcs = funcs;
>
> If KLP_OBSOLETES parsing fails during string duplication, array allocation,
> or integer conversion, the error paths jump to err_free_objs. It appears
> that the cleanup label only frees the objs array itself but leaves the
> individual obj->funcs arrays allocated:
>
> err_free_objs:
>         kfree(objs);
> err_free_patch:
>         kfree(patch);

You are right.
Furthermore, there is a pre-existing bug in the error path that should
be fixed as well:

--- a/scripts/livepatch/init.c
+++ b/scripts/livepatch/init.c
@@ -52,7 +52,7 @@ static int __init livepatch_mod_init(void)
                if (!funcs) {
                        ret = -ENOMEM;
                        for (int j = 0; j < i; j++)
-                               kfree(objs[i].funcs);
+                               kfree(objs[j].funcs);
                        goto err_free_objs;
                }

-- 
Regards
Yafang