Message-ID: <7e023c28-abe5-25a6-150d-ff8a782992de@huawei.com>
Date:   Fri, 28 Apr 2023 09:05:32 +0800
From:   Chen Zhongjin <chenzhongjin@...wei.com>
To:     Yonghong Song <yhs@...a.com>, <bpf@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
CC:     <ast@...nel.org>, <daniel@...earbox.net>,
        <john.fastabend@...il.com>, <andrii@...nel.org>,
        <martin.lau@...ux.dev>, <song@...nel.org>, <yhs@...com>,
        <kpsingh@...nel.org>, <sdf@...gle.com>, <haoluo@...gle.com>,
        <jolsa@...nel.org>
Subject: Re: [PATCH] bpf: Unregister fentry when bpf_trampoline_unlink_prog
 fails to update image



On 2023/4/28 2:54, Yonghong Song wrote:
> 
> ...
> 
>>
>>>>
>>>> Fix this when bpf_trampoline_update failed in 
>>>> bpf_trampoline_unlink_prog,
>>>> unregister fentry to disable the trampoline. Then other progs on the
>>>> trampoline can be unlinked safely and finally the trampoline will be
>>>> released.
>>>
>>> Do we still leak tr->cur_image here?
>>>
>> No, bpf_tramp_image_put() will free everything when all progs_cnt 
>> decline to zero in bpf_trampoline_update(). It is a release function, 
>> but called 'put'.
> 
> Okay, I see. But it depends on where memory allocation failure
> happens. For example, see below:
> 
>      static int bpf_trampoline_update(struct bpf_trampoline *tr,
>      bool lock_direct_mutex)
>      {
>          struct bpf_tramp_image *im;
>          struct bpf_tramp_links *tlinks;
>          u32 orig_flags = tr->flags;
>          bool ip_arg = false;
>          int err, total;
> 
>          tlinks = bpf_trampoline_get_progs(tr, &total, &ip_arg);
>          if (IS_ERR(tlinks))
>                  return PTR_ERR(tlinks);
> 
>          if (total == 0) {
>                  err = unregister_fentry(tr, tr->cur_image->image);
>                  bpf_tramp_image_put(tr->cur_image);
>                  tr->cur_image = NULL;
>                  tr->selector = 0;
>                  goto out;
>          }
> 
>          im = bpf_tramp_image_alloc(tr->key, tr->selector);
>          if (IS_ERR(im)) {
>                  err = PTR_ERR(im);
>                  goto out;
>          }
> 
>      ...
> 
> In bpf_trampoline_get_progs(),
> 
>      static struct bpf_tramp_links *
>      bpf_trampoline_get_progs(const struct bpf_trampoline *tr, int 
> *total, bool *ip_arg)
>      {
>          struct bpf_tramp_link *link;
>          struct bpf_tramp_links *tlinks;
>          struct bpf_tramp_link **links;
>          int kind;
> 
>          *total = 0;
>          tlinks = kcalloc(BPF_TRAMP_MAX, sizeof(*tlinks), GFP_KERNEL);
> 
> If we have memory allocation failure, PTR_ERR(tlinks) will be returned
> and there is no chance to reach 'total == 0' so tr->cur_image will not
> be freed.
> 
> But if the memory allocation failure happens in 
> bpf_tramp_image_alloc(...), yes, it is possible eventually when
> all progs are unlinked, 'total' could be 0, unregister_fentry()
> will be called again (hopefully no side effect) and the image
> will be freed.
> 
>>
>>>>
>>>> Fixes: 88fd9e5352fe ("bpf: Refactor trampoline update code")
>>>
>>> If the above is a refactoring patch, you should not use that
>>> as 'Fixes' patch, you should pick one truely introduced the issue.
>>>
>>>> Signed-off-by: Chen Zhongjin <chenzhongjin@...wei.com>
>>>> ---
>>>>   kernel/bpf/trampoline.c | 5 ++++-
>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
>>>> index d0ed7d6f5eec..6daa93b30e81 100644
>>>> --- a/kernel/bpf/trampoline.c
>>>> +++ b/kernel/bpf/trampoline.c
>>>> @@ -604,7 +604,10 @@ static int __bpf_trampoline_unlink_prog(struct 
>>>> bpf_tramp_link *link, struct bpf_
>>>>       }
>>>>       hlist_del_init(&link->tramp_hlist);
>>>>       tr->progs_cnt[kind]--;
>>>> -    return bpf_trampoline_update(tr, true /* lock_direct_mutex */);
>>>> +    err =  bpf_trampoline_update(tr, true /* lock_direct_mutex */);
>>>> +    if (err && tr->cur_image)
>>>> +        unregister_fentry(tr, tr->cur_image->image);
>>>
>>> If error happens for the all subsequent programs,
>>> unregister_fentry() will be called multiple times. Any side effect?
>> It will fail with no side effect. Actually if there is no error, 
>> modify_fentry() will fail in update() as well. The fentry is available 
>> until all progs are unlinked and the broken image is freed by 
>> bpf_tramp_image_put().
> 
> This will cause a sudden behavior change if there are other active
> programs. For example, prog1 and prog2, prog1 unlink triggered
> memory allocation failure and caused unregister_fentry which restored
> the original ip. Then prog2 even does not have a chance to run any
> more even if memory suddenly not an issue any more.
> 
>>
>> However with an extra state to record this happens, it's possible to 
>> re-register the fentry with new image when the next link/unlink calls 
>> update(). It will generate a new image and replace/free the error one.
>>>
>>> Overall, I think this is an extreme corner case which happens
>>> when kernel memory is extreme tight. If this is the case, not
>>> sure whether it is worthwhile to fix it or not.
>>>
>> Yes, it's a really rare case. I'm just not sure whether it needs some 
>> best-effort to avoid kernel panic at this point.
>>
>> If you think it's not necessary. Just let it go.
> 
> At least with current patch, even we could avoid kernel panic, the
> user expected behavior might change. To handle all error conditions
> in bpf_trampoline_update() might be a big task. So I suggest to just
> let it go.
> 
You are right. It's difficult to handle all errors, especially when
problem occurs inside the ftrace process.
So just keep it as is until there is better solution.

>>
>> Thanks for your time!
>>
>> Best,
>> Chen
>>>> +    return err;
>>>>   }
>>>>   /* bpf_trampoline_unlink_prog() should never fail. */

Powered by blists - more mailing lists