netdev - Re: [RFC bpf-next 5/5] samples/bpf/xdpsock_user.c: Launchtime/TXTIME API usage

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]

Date:   Thu, 19 Aug 2021 15:32:35 -0400
From:   Kishen Maloor <kishen.maloor@...el.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>, <bpf@...r.kernel.org>,
        <netdev@...r.kernel.org>, <hawk@...nel.org>,
        <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn@...nel.org>
CC:     <brouer@...hat.com>, Jithu Joseph <jithu.joseph@...el.com>
Subject: Re: [RFC bpf-next 5/5] samples/bpf/xdpsock_user.c: Launchtime/TXTIME
 API usage

On 8/18/21 4:54 AM, Jesper Dangaard Brouer wrote:
> 
> On 03/08/2021 19.10, Kishen Maloor wrote:
>> diff --git a/samples/bpf/xdpsock_user.c b/samples/bpf/xdpsock_user.c
>> index 3fd2f6a0d1eb..a0fd3d5414ba 100644
>> --- a/samples/bpf/xdpsock_user.c
>> +++ b/samples/bpf/xdpsock_user.c
> [...]
>> @@ -741,6 +745,8 @@ static inline u16 udp_csum(u32 saddr, u32 daddr, u32 len,
>>     #define ETH_FCS_SIZE 4
>>   +#define MD_SIZE (sizeof(struct xdp_user_tx_metadata))
>> +
>>   #define PKT_HDR_SIZE (sizeof(struct ethhdr) + sizeof(struct iphdr) + \
>>                 sizeof(struct udphdr))
>>   @@ -798,8 +804,10 @@ static void gen_eth_hdr_data(void)
>>     static void gen_eth_frame(struct xsk_umem_info *umem, u64 addr)
>>   {
>> -    memcpy(xsk_umem__get_data(umem->buffer, addr), pkt_data,
>> -           PKT_SIZE);
>> +    if (opt_launch_time)
>> +        memcpy(xsk_umem__get_data(umem->buffer, addr) + MD_SIZE, pkt_data, PKT_SIZE);
>> +    else
>> +        memcpy(xsk_umem__get_data(umem->buffer, addr), pkt_data, PKT_SIZE);
>>   }
>>   
> 
> I imagined that AF_XDP 'addr' would still point to the start of the packet data, and that metadata area was access via a negative offset from 'addr'.
> 

There is currently no kernel "infrastructure" on the TX path which factors in the concept of XDP metadata, so the application needs to make place for it. (For e.g., XDP_PACKET_HEADROOM has no de facto role on the TX path AFAIK).

xsk_umem__get_data() just returns the UMEM chunk at the user supplied 'addr' and applications need to write both the XDP packet and any accompanying metadata into this (raw) buffer.

In doing so, it places that metadata right ahead of the XDP packet (much like how that's structured in the RX path), and further plugs (addr + offset_to_XDP_packet (the metadata size, in other words)) into the TX descriptor 'addr' so that lower layers (e.g. the driver) can access the XDP packet as always.
(Note also that the TX descriptor 'len' would exclude the md size)

> Maybe I misunderstood the code, but it looks like 'addr' (xsk_umem__get_data(umem->buffer, addr)) points to metadata area, is this correct?
> 

No, more specifically, it is the user supplied UMEM chunk 'addr'. 

> (and to skip this the code does + MD_SIZE, before memcpy)

Since the packet is written (by the application) to immediately follow the metadata (both stored on the chunk), the code does that (+ MD_SIZE).

> 
> One problem/challenge with AF_XDP is that we don't have room in struct xdp_desc to store info on the size of the metadata area.  Bjørn came up with the idea of having btf_id as last member (access able via minus 4 bytes), as this tells the kernel the size of metadata area.

Yes, the RFC follows this idea and hence btf_id is the last member of struct xdp_user_tx_metadata.

> 
> Maybe you have come up with a better solution?
> (of making the metadata area size dynamic)
> 
> --Jesper
>