Hi Dan,
On Wed, Oct 25, 2023 at 2:40 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Yan Zhai,
>
> The patch 29b22badb7a8: "lwt: Fix return values of BPF xmit ops" from
> Aug 17, 2023 (linux-next), leads to the following Smatch static
> checker warning:
>
> net/core/lwt_bpf.c:131 bpf_input()
> error: double free of 'skb'
>
Thanks for reporting. I looked at the code, and it is possible to
continue processing skb on bpf_input and bpf_output when BPF_REDIRECT
is returned. However, both paths call run_lwt_bpf with NO_REDIRECT as
can_redirect bool arg, which means the skb_do_redirect branch won't
trigger. So it does not look like a bug to me. The life-cycle of skb
is a bit messy around this corner though.
Yan
> net/core/lwt_bpf.c
> 38 static int run_lwt_bpf(struct sk_buff *skb, struct bpf_lwt_prog *lwt,
> 39 struct dst_entry *dst, bool can_redirect)
> 40 {
> 41 int ret;
> 42
> 43 /* Migration disable and BH disable are needed to protect per-cpu
> 44 * redirect_info between BPF prog and skb_do_redirect().
> 45 */
> 46 migrate_disable();
> 47 local_bh_disable();
> 48 bpf_compute_data_pointers(skb);
> 49 ret = bpf_prog_run_save_cb(lwt->prog, skb);
> 50
> 51 switch (ret) {
> 52 case BPF_OK:
> 53 case BPF_LWT_REROUTE:
> 54 break;
> 55
> 56 case BPF_REDIRECT:
> 57 if (unlikely(!can_redirect)) {
> 58 pr_warn_once("Illegal redirect return code in prog %s\n",
> 59 lwt->name ? : "<unknown>");
> 60 ret = BPF_OK;
> 61 } else {
> 62 skb_reset_mac_header(skb);
> 63 skb_do_redirect(skb);
> 64 ret = BPF_REDIRECT;
>
> If skb_do_redirect() returns -EINVAL it means the skb has been freed.
> Originally we preserved error code but now we just return BPF_REDIRECT.
>
> 65 }
> 66 break;
> 67
> 68 case BPF_DROP:
> 69 kfree_skb(skb);
> 70 ret = -EPERM;
> 71 break;
>
> regards,
> dan carpenter
