From: Daniel Borkmann <daniel@iogearbox•net>
To: Martin KaFai Lau <kafai@fb•com>,
Jesper Dangaard Brouer <brouer@redhat•com>
Cc: netdev@vger•kernel.org, Alexei Starovoitov <ast@fb•com>,
Brenden Blanco <bblanco@plumgrid•com>,
David Miller <davem@davemloft•net>,
Saeed Mahameed <saeedm@mellanox•com>,
Tariq Toukan <tariqt@mellanox•com>,
Kernel Team <kernel-team@fb•com>
Subject: Re: [PATCH net-next 1/4] bpf: xdp: Allow head adjustment in XDP prog
Date: Sat, 03 Dec 2016 21:21:01 +0100 [thread overview]
Message-ID: <5843292D.5030302@iogearbox.net> (raw)
In-Reply-To: <20161203193249.GC70461@kafai-mba.local>
On 12/03/2016 08:32 PM, Martin KaFai Lau wrote:
> On Sat, Dec 03, 2016 at 04:24:13PM +0100, Jesper Dangaard Brouer wrote:
>> On Fri, 2 Dec 2016 15:23:30 -0800
>> Martin KaFai Lau <kafai@fb•com> wrote:
>>
>>> -bool bpf_helper_changes_skb_data(void *func)
>>> +BPF_CALL_2(bpf_xdp_adjust_head, struct xdp_buff *, xdp, int, offset)
>>> +{
>>> + /* Both mlx4 and mlx5 driver align each packet to PAGE_SIZE when
>>> + * XDP prog is set.
>>> + * If the above is not true for the other drivers to support
>>> + * bpf_xdp_adjust_head, struct xdp_buff can be extended.
>>> + */
>>> + void *head = (void *)((unsigned long)xdp->data & PAGE_MASK);
>>> + void *new_data = xdp->data + offset;
>>> +
>>> + if (new_data < head || new_data >= xdp->data_end)
>>> + /* The packet length must be >=1 */
>>> + return -EINVAL;
>>> +
>>> + xdp->data = new_data;
>>> +
>>> + return 0;
>>> +}
>>
>> First time I read this code, I was about to complain about you didn't
>> use XDP_PACKET_HEADROOM in your boundary check. But then I noticed the
>> PAGE_MASK. If you rename "head" to "page_boundary" or "page_start"
>> then IMHO the code would be more readable.
> bpf_xdp_adjust_head() could be called multiple times. Hence,
> XDP_PACKET_HEADROOM is not used in the boundary check.
>
> My thinking is "head" here can closely resemble the meaning of
> skb->head as a boundary. I think missing the info on
> what head it is could be the confusing part.
>
> Instead of skb boundary (there is no skb here) or
> page boundary (other future XDP driver may not align like mlx4/5),
> I think may be "pkt_head" can give more clarity here and also
> for furture XDP-capble driver?
I think as-is with head is also fine with me, but if it should be
something better readable (?), perhaps as such (modulo the min len
part):
BPF_CALL_2(bpf_xdp_adjust_head, struct xdp_buff *, xdp, int, offset)
{
unsigned long addr = (unsigned long)xdp->data & PAGE_MASK;
void *data_hard_start = (void *)addr;
void *data = xdp->data + offset;
if (unlikely(data < data_hard_start || data >= xdp->data_end))
return -EINVAL;
xdp->data = data;
return 0;
}
Thanks,
Daniel
next prev parent reply other threads:[~2016-12-03 20:21 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-02 23:23 [PATCH net-next 0/4]: Allow head adjustment in XDP prog Martin KaFai Lau
2016-12-02 23:23 ` [PATCH net-next 1/4] bpf: xdp: " Martin KaFai Lau
2016-12-03 0:22 ` Daniel Borkmann
2016-12-03 3:43 ` Martin KaFai Lau
2016-12-03 15:24 ` Jesper Dangaard Brouer
2016-12-03 19:32 ` Martin KaFai Lau
2016-12-03 20:21 ` Daniel Borkmann [this message]
2016-12-02 23:23 ` [PATCH net-next 2/4] mlx4: xdp: Allow raising MTU up to one page minus eth and vlan hdrs Martin KaFai Lau
2016-12-03 0:07 ` Rick Jones
2016-12-04 3:19 ` Martin KaFai Lau
2016-12-05 17:46 ` Jakub Kicinski
2016-12-03 0:38 ` Eric Dumazet
2016-12-03 0:53 ` Alexei Starovoitov
2016-12-03 2:15 ` Eric Dumazet
2016-12-03 3:42 ` Martin KaFai Lau
2016-12-03 3:59 ` Eric Dumazet
2016-12-04 23:31 ` Saeed Mahameed
2016-12-05 0:52 ` Eric Dumazet
2016-12-02 23:23 ` [PATCH net-next 3/4] mlx4: xdp: Reserve headroom for receiving packet when XDP prog is active Martin KaFai Lau
2016-12-02 23:23 ` [PATCH net-next 4/4] bpf: xdp: Add XDP example for head adjustment Martin KaFai Lau
2016-12-03 15:17 ` [PATCH net-next 0/4]: Allow head adjustment in XDP prog Jesper Dangaard Brouer
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=5843292D.5030302@iogearbox.net \
--to=daniel@iogearbox$(echo .)net \
--cc=ast@fb$(echo .)com \
--cc=bblanco@plumgrid$(echo .)com \
--cc=brouer@redhat$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=kafai@fb$(echo .)com \
--cc=kernel-team@fb$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=saeedm@mellanox$(echo .)com \
--cc=tariqt@mellanox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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