netdev - Re: [PATCH bpf-next,v4 2/4] xfrm: interface: Add unstable helpers for setting/getting XFRM metadata from TC-BPF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHsH6Gtt4vihaZ5kCFsjT8x1SmuiUkijnVxgAA9bMp4NOgPeAw@mail.gmail.com>
Date:   Fri, 2 Dec 2022 21:42:10 +0200
From:   Eyal Birger <eyal.birger@...il.com>
To:     Martin KaFai Lau <martin.lau@...ux.dev>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org,
        linux-kselftest@...r.kernel.org, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        steffen.klassert@...unet.com, herbert@...dor.apana.org.au,
        andrii@...nel.org, daniel@...earbox.net, nicolas.dichtel@...nd.com,
        razor@...ckwall.org, mykolal@...com, ast@...nel.org,
        song@...nel.org, yhs@...com, john.fastabend@...il.com,
        kpsingh@...nel.org, sdf@...gle.com, haoluo@...gle.com,
        jolsa@...nel.org, shuah@...nel.org, liuhangbin@...il.com,
        lixiaoyan@...gle.com
Subject: Re: [PATCH bpf-next,v4 2/4] xfrm: interface: Add unstable helpers for
 setting/getting XFRM metadata from TC-BPF

Hi Martin,

On Fri, Dec 2, 2022 at 9:08 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 12/2/22 1:59 AM, Eyal Birger wrote:
> > +__used noinline
> > +int bpf_skb_set_xfrm_info(struct __sk_buff *skb_ctx,
> > +                       const struct bpf_xfrm_info *from)
> > +{
> > +     struct sk_buff *skb = (struct sk_buff *)skb_ctx;
> > +     struct metadata_dst *md_dst;
> > +     struct xfrm_md_info *info;
> > +
> > +     if (unlikely(skb_metadata_dst(skb)))
> > +             return -EINVAL;
> > +
> > +     md_dst = this_cpu_ptr(xfrm_md_dst);
> > +
> > +     info = &md_dst->u.xfrm_info;
> > +
> > +     info->if_id = from->if_id;
> > +     info->link = from->link;
> > +     skb_dst_force(skb);
> > +     info->dst_orig = skb_dst(skb);
> > +
> > +     dst_hold((struct dst_entry *)md_dst);
> > +     skb_dst_set(skb, (struct dst_entry *)md_dst);
>
>
> I may be missed something obvious and this just came to my mind,
>
> What stops cleanup_xfrm_interface_bpf() being run while skb is still holding the
> md_dst?
>
Oh I think you're right. I missed this.

In order to keep this implementation I suppose it means that the module would
not be allowed to be removed upon use of this kfunc. but this could be seen as
annoying from the configuration user experience.

Alternatively the metadata dsts can be separately allocated from the kfunc,
which is probably the simplest approach to maintain, so I'll work on that
approach.

Thanks for noticing this!
Eyal.