From: Eyal Birger <eyal.birger@gmail.com>
To: Martin KaFai Lau <martin.lau@linux.dev>
Cc: netdev@vger.kernel.org, bpf@vger.kernel.org,
linux-kselftest@vger.kernel.org, davem@davemloft.net,
edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
steffen.klassert@secunet.com, herbert@gondor.apana.org.au,
andrii@kernel.org, daniel@iogearbox.net,
nicolas.dichtel@6wind.com, razor@blackwall.org, mykolal@fb.com,
ast@kernel.org, song@kernel.org, yhs@fb.com,
john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
haoluo@google.com, jolsa@kernel.org, shuah@kernel.org,
liuhangbin@gmail.com, lixiaoyan@google.com
Subject: Re: [PATCH bpf-next,v4 2/4] xfrm: interface: Add unstable helpers for setting/getting XFRM metadata from TC-BPF
Date: Fri, 2 Dec 2022 22:49:37 +0200 [thread overview]
Message-ID: <CAHsH6Gt=WQhcqTsrDRhVyOSMwc4be5JaY9LpkbtFunvNZx3_Cg@mail.gmail.com> (raw)
In-Reply-To: <4cf2ecd4-2f21-848a-00df-4e4fd86667eb@linux.dev>
On Fri, Dec 2, 2022 at 10:27 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 12/2/22 11:42 AM, Eyal Birger wrote:
> > Hi Martin,
> >
> > On Fri, Dec 2, 2022 at 9:08 PM Martin KaFai Lau <martin.lau@linux.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.
>
> If it means dst_alloc on every skb, it will not be cheap.
>
> Another option is to metadata_dst_alloc_percpu() once during the very first
> bpf_skb_set_xfrm_info() call and the xfrm_md_dst memory will never be freed. It
> is a tradeoff but likely the correct one. You can take a look at
> bpf_get_skb_set_tunnel_proto().
>
Yes, I originally wrote this as a helper similar to the tunnel key
helper which uses bpf_get_skb_set_tunnel_proto(), and when converting
to kfuncs I kept the
percpu implementation.
However, the set tunnel key code is never unloaded. Whereas taking this
approach here would mean that this memory would leak on each module reload
iiuc.
Eyal.
next prev parent reply other threads:[~2022-12-02 20:51 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-02 9:59 [PATCH bpf-next,v4 0/4] xfrm: interface: Add unstable helpers for XFRM metadata Eyal Birger
2022-12-02 9:59 ` [PATCH bpf-next,v4 1/4] xfrm: interface: rename xfrm_interface.c to xfrm_interface_core.c Eyal Birger
2022-12-02 9:59 ` [PATCH bpf-next,v4 2/4] xfrm: interface: Add unstable helpers for setting/getting XFRM metadata from TC-BPF Eyal Birger
2022-12-02 19:08 ` Martin KaFai Lau
2022-12-02 19:42 ` Eyal Birger
2022-12-02 20:27 ` Martin KaFai Lau
2022-12-02 20:49 ` Eyal Birger [this message]
2022-12-02 21:27 ` Martin KaFai Lau
2022-12-03 3:55 ` Eyal Birger
2022-12-03 7:35 ` Eyal Birger
2022-12-02 9:59 ` [PATCH bpf-next,v4 3/4] tools: add IFLA_XFRM_COLLECT_METADATA to uapi/linux/if_link.h Eyal Birger
2022-12-02 9:59 ` [PATCH bpf-next,v4 4/4] selftests/bpf: add xfrm_info tests Eyal Birger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHsH6Gt=WQhcqTsrDRhVyOSMwc4be5JaY9LpkbtFunvNZx3_Cg@mail.gmail.com' \
--to=eyal.birger@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=haoluo@google.com \
--cc=herbert@gondor.apana.org.au \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=liuhangbin@gmail.com \
--cc=lixiaoyan@google.com \
--cc=martin.lau@linux.dev \
--cc=mykolal@fb.com \
--cc=netdev@vger.kernel.org \
--cc=nicolas.dichtel@6wind.com \
--cc=pabeni@redhat.com \
--cc=razor@blackwall.org \
--cc=sdf@google.com \
--cc=shuah@kernel.org \
--cc=song@kernel.org \
--cc=steffen.klassert@secunet.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).