From: Daniel Borkmann <daniel@iogearbox.net>
To: Martin KaFai Lau <kafai@fb.com>
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org,
Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
David Miller <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>,
kernel-team@fb.com, Willem de Bruijn <willemb@google.com>
Subject: Re: [PATCH v6 net-next 11/13] bpf: Keep the (rcv) timestamp behavior for the existing tc-bpf@ingress
Date: Thu, 3 Mar 2022 23:55:55 +0100 [thread overview]
Message-ID: <419d994e-ff61-7c11-0ec7-11fefcb0186e@iogearbox.net> (raw)
In-Reply-To: <20220303204303.bpqlpbyylodpax5x@kafai-mbp>
On 3/3/22 9:43 PM, Martin KaFai Lau wrote:
> On Thu, Mar 03, 2022 at 02:00:37PM +0100, Daniel Borkmann wrote:
>> On 3/2/22 8:56 PM, Martin KaFai Lau wrote:
>>> If the tc-bpf@egress writes 0 to skb->tstamp, the skb->mono_delivery_time
>>> has to be cleared also. It could be done together during
>>> convert_ctx_access(). However, the latter patch will also expose
>>> the skb->mono_delivery_time bit as __sk_buff->delivery_time_type.
>>> Changing the delivery_time_type in the background may surprise
>>> the user, e.g. the 2nd read on __sk_buff->delivery_time_type
>>> may need a READ_ONCE() to avoid compiler optimization. Thus,
>>> in expecting the needs in the latter patch, this patch does a
>>> check on !skb->tstamp after running the tc-bpf and clears the
>>> skb->mono_delivery_time bit if needed. The earlier discussion
>>> on v4 [0].
>
> [ ... ]
>
>>> @@ -1047,10 +1047,16 @@ struct sk_buff {
>>> /* if you move pkt_vlan_present around you also must adapt these constants */
>>> #ifdef __BIG_ENDIAN_BITFIELD
>>> #define PKT_VLAN_PRESENT_BIT 7
>>> +#define TC_AT_INGRESS_MASK (1 << 0)
>>> +#define SKB_MONO_DELIVERY_TIME_MASK (1 << 2)
>>> #else
>>> #define PKT_VLAN_PRESENT_BIT 0
>>> +#define TC_AT_INGRESS_MASK (1 << 7)
>>> +#define SKB_MONO_DELIVERY_TIME_MASK (1 << 5)
>>> #endif
>>> #define PKT_VLAN_PRESENT_OFFSET offsetof(struct sk_buff, __pkt_vlan_present_offset)
>>> +#define TC_AT_INGRESS_OFFSET offsetof(struct sk_buff, __pkt_vlan_present_offset)
>>> +#define SKB_MONO_DELIVERY_TIME_OFFSET offsetof(struct sk_buff, __pkt_vlan_present_offset)
>>
>> Just nit, but given PKT_VLAN_PRESENT_OFFSET, TC_AT_INGRESS_OFFSET and SKB_MONO_DELIVERY_TIME_OFFSET
>> are all the same offsetof(struct sk_buff, __pkt_vlan_present_offset), maybe lets use just one single
>> define? If anyone moves them out, they would have to adopt as per comment.
> Make sense. I will update the comment, remove these two defines
> and reuse the PKT_VLAN_PRESENT_OFFSET. Considering it
> is more bpf insn rewrite specific, I will do a
> follow-up in filter.c and skbuff.h at bpf-next.
Ok, sounds good!
>>> #ifdef __KERNEL__
>>> /*
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index cfcf9b4d1ec2..5072733743e9 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -8859,6 +8859,65 @@ static struct bpf_insn *bpf_convert_shinfo_access(const struct bpf_insn *si,
>>> return insn;
>>> }
>>> +static struct bpf_insn *bpf_convert_tstamp_read(const struct bpf_insn *si,
>>> + struct bpf_insn *insn)
>>> +{
>>> + __u8 value_reg = si->dst_reg;
>>> + __u8 skb_reg = si->src_reg;
>>> +
>>> +#ifdef CONFIG_NET_CLS_ACT
>>> + __u8 tmp_reg = BPF_REG_AX;
>>> +
>>> + *insn++ = BPF_LDX_MEM(BPF_B, tmp_reg, skb_reg, TC_AT_INGRESS_OFFSET);
>>> + *insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg, TC_AT_INGRESS_MASK);
>>
>> nit: As far as I can see, can't si->dst_reg be used instead of AX?
> Ah. This one got me also when using dst_reg as a tmp. dst_reg and src_reg
> can be the same:
>
> ; skb->tstamp == EGRESS_FWDNS_MAGIC)
> 169: r1 = *(u64 *)(r1 + 152)
Ah true, good point. Probably makes sense to add a small comment explaining
the rationale for use of AX here.
>>> + *insn++ = BPF_JMP32_IMM(BPF_JEQ, tmp_reg, 0, 5);
>>> + /* @ingress, read __sk_buff->tstamp as the (rcv) timestamp,
>>> + * so check the skb->mono_delivery_time.
>>> + */
>>> + *insn++ = BPF_LDX_MEM(BPF_B, tmp_reg, skb_reg,
>>> + SKB_MONO_DELIVERY_TIME_OFFSET);
>>> + *insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg,
>>> + SKB_MONO_DELIVERY_TIME_MASK);
>>> + *insn++ = BPF_JMP32_IMM(BPF_JEQ, tmp_reg, 0, 2);
>>> + /* skb->mono_delivery_time is set, read 0 as the (rcv) timestamp. */
>>> + *insn++ = BPF_MOV64_IMM(value_reg, 0);
>>> + *insn++ = BPF_JMP_A(1);
>>> +#endif
>>> +
>>> + *insn++ = BPF_LDX_MEM(BPF_DW, value_reg, skb_reg,
>>> + offsetof(struct sk_buff, tstamp));
>>> + return insn;
>>> +}
>>> +
>>> +static struct bpf_insn *bpf_convert_tstamp_write(const struct bpf_insn *si,
>>> + struct bpf_insn *insn)
>>> +{
>>> + __u8 value_reg = si->src_reg;
>>> + __u8 skb_reg = si->dst_reg;
>>> +
>>> +#ifdef CONFIG_NET_CLS_ACT
>>> + __u8 tmp_reg = BPF_REG_AX;
>>> +
>>> + *insn++ = BPF_LDX_MEM(BPF_B, tmp_reg, skb_reg, TC_AT_INGRESS_OFFSET);
>>> + *insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg, TC_AT_INGRESS_MASK);
>>
>> Can't we get rid of tcf_bpf_act() and cls_bpf_classify() changes altogether by just doing:
>>
>> /* BPF_WRITE: __sk_buff->tstamp = a */
>> skb->mono_delivery_time = !skb->tc_at_ingress && a;
>> skb->tstamp = a;
> It will then assume the bpf prog is writing a mono time.
> Although mono should always be the case now, this assumption will be
> an issue in the future if we need to support non-mono.
Right, for that we should probably instrument verifier to track base based
on ktime helper call once we get to that point.
>> (Untested) pseudo code:
>>
>> // or see comment on common SKB_FLAGS_OFFSET define or such
>> BUILD_BUG_ON(TC_AT_INGRESS_OFFSET != SKB_MONO_DELIVERY_TIME_OFFSET)
>>
>> BPF_LDX_MEM(BPF_B, tmp_reg, skb_reg, SKB_MONO_DELIVERY_TIME_OFFSET)
>> BPF_ALU32_IMM(BPF_OR, tmp_reg, SKB_MONO_DELIVERY_TIME_MASK)
>> BPF_JMP32_IMM(BPF_JSET, tmp_reg, TC_AT_INGRESS_MASK, <clear>)
> This can save a BPF_ALU32_IMM(BPF_AND). I will do that
> together in the follow up. Thanks for the idea !
Yeah the JSET comes in handy here.
>> BPF_JMP32_REG(BPF_JGE, value_reg, tmp_reg, <store>)
>> <clear>:
>> BPF_ALU32_IMM(BPF_AND, tmp_reg, ~SKB_MONO_DELIVERY_TIME_MASK)
>> <store>:
>> BPF_STX_MEM(BPF_B, skb_reg, tmp_reg, SKB_MONO_DELIVERY_TIME_OFFSET)
>> BPF_STX_MEM(BPF_DW, skb_reg, value_reg, offsetof(struct sk_buff, tstamp))
>>
>> (There's a small hack with the BPF_JGE for tmp_reg, so constant blinding for AX doesn't
>> get into our way.)
>>
>>> + *insn++ = BPF_JMP32_IMM(BPF_JEQ, tmp_reg, 0, 3);
>>> + /* Writing __sk_buff->tstamp at ingress as the (rcv) timestamp.
>>> + * Clear the skb->mono_delivery_time.
>>> + */
>>> + *insn++ = BPF_LDX_MEM(BPF_B, tmp_reg, skb_reg,
>>> + SKB_MONO_DELIVERY_TIME_OFFSET);
>>> + *insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg,
>>> + ~SKB_MONO_DELIVERY_TIME_MASK);
>>> + *insn++ = BPF_STX_MEM(BPF_B, skb_reg, tmp_reg,
>>> + SKB_MONO_DELIVERY_TIME_OFFSET);
>>> +#endif
>>> +
>>> + /* skb->tstamp = tstamp */
>>> + *insn++ = BPF_STX_MEM(BPF_DW, skb_reg, value_reg,
>>> + offsetof(struct sk_buff, tstamp));
>>> + return insn;
>>> +}
>>> +
next prev parent reply other threads:[~2022-03-03 22:56 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-02 19:55 [PATCH v6 net-next 0/13] Preserve mono delivery time (EDT) in skb->tstamp Martin KaFai Lau
2022-03-02 19:55 ` [PATCH v6 net-next 01/13] net: Add skb->mono_delivery_time to distinguish mono delivery_time from (rcv) timestamp Martin KaFai Lau
2022-03-02 19:55 ` [PATCH v6 net-next 02/13] net: Add skb_clear_tstamp() to keep the mono delivery_time Martin KaFai Lau
2022-03-02 19:55 ` [PATCH v6 net-next 03/13] net: Handle delivery_time in skb->tstamp during network tapping with af_packet Martin KaFai Lau
2022-03-03 10:48 ` Daniel Borkmann
2022-03-03 19:17 ` Martin KaFai Lau
2022-03-02 19:55 ` [PATCH v6 net-next 04/13] net: Clear mono_delivery_time bit in __skb_tstamp_tx() Martin KaFai Lau
2022-03-02 19:55 ` [PATCH v6 net-next 05/13] net: Set skb->mono_delivery_time and clear it after sch_handle_ingress() Martin KaFai Lau
2022-03-02 19:55 ` [PATCH v6 net-next 06/13] net: ip: Handle delivery_time in ip defrag Martin KaFai Lau
2022-03-02 19:56 ` [PATCH v6 net-next 07/13] net: ipv6: Handle delivery_time in ipv6 defrag Martin KaFai Lau
2022-03-02 19:56 ` [PATCH v6 net-next 08/13] net: ipv6: Get rcv timestamp if needed when handling hop-by-hop IOAM option Martin KaFai Lau
2022-03-02 19:56 ` [PATCH v6 net-next 09/13] net: Get rcv tstamp if needed in nfnetlink_{log, queue}.c Martin KaFai Lau
2022-03-02 19:56 ` [PATCH v6 net-next 10/13] net: Postpone skb_clear_delivery_time() until knowing the skb is delivered locally Martin KaFai Lau
2022-03-02 20:30 ` Eric Dumazet
2022-03-02 22:33 ` Martin KaFai Lau
2022-03-02 23:41 ` Eric Dumazet
2022-03-03 0:19 ` Martin KaFai Lau
2022-03-03 0:47 ` Eric Dumazet
2022-03-02 19:56 ` [PATCH v6 net-next 11/13] bpf: Keep the (rcv) timestamp behavior for the existing tc-bpf@ingress Martin KaFai Lau
2022-03-03 13:00 ` Daniel Borkmann
2022-03-03 20:43 ` Martin KaFai Lau
2022-03-03 22:55 ` Daniel Borkmann [this message]
2022-03-03 23:42 ` Martin KaFai Lau
2022-03-02 19:56 ` [PATCH v6 net-next 12/13] bpf: Add __sk_buff->delivery_time_type and bpf_skb_set_skb_delivery_time() Martin KaFai Lau
2022-03-02 19:56 ` [PATCH v6 net-next 13/13] bpf: selftests: test skb->tstamp in redirect_neigh Martin KaFai Lau
2022-03-03 0:36 ` [PATCH v6 net-next 0/13] Preserve mono delivery time (EDT) in skb->tstamp Eric Dumazet
2022-03-03 14:50 ` patchwork-bot+netdevbpf
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=419d994e-ff61-7c11-0ec7-11fefcb0186e@iogearbox.net \
--to=daniel@iogearbox.net \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kafai@fb.com \
--cc=kernel-team@fb.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=willemb@google.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).