[ovs-dev] [PATCH] netdev-afxdp: add afxdp specific maximum MTU check

[Ilya Maximets]

On 07.11.2019 15:01, Eelco Chaudron wrote:
> Any feedback on this?
> 
> 
> On 1 Oct 2019, at 11:55, Eelco Chaudron wrote:
> 
>> Drivers natively supporting AF_XDP will check that a configured MTU size
>> will not exceed the allowed size for AF_XDP. However, when the skb
>> compatibility mode is used there is no check and any value is accepted.

This sounds like a kernel issue.
So, maybe it's better to fix it there?

>> This, for example, is the case when using the TAP interface.
>>
>> This fix adds a check to make sure only AF_XDP valid values are excepted.
>>
>> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
>> ---
>>  lib/netdev-afxdp.c |   17 +++++++++++++++++
>>  lib/netdev-afxdp.h |    1 +
>>  lib/netdev-linux.c |    9 +++++++++
>>  3 files changed, 27 insertions(+)
>>
>> diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
>> index 6e0180327..140150f29 100644
>> --- a/lib/netdev-afxdp.c
>> +++ b/lib/netdev-afxdp.c
>> @@ -1001,6 +1001,23 @@ netdev_afxdp_destruct(struct netdev *netdev)
>>      ovs_mutex_destroy(&dev->mutex);
>>  }
>>
>> +int
>> +netdev_afxdp_verify_mtu_size(const struct netdev *netdev, int mtu)
>> +{
>> +    /*
>> +     * If a device is used in xdpmode skb, no driver-specific MTU size is
>> +     * checked and any value is allowed resulting in packet drops.
>> +     * This check will verify the maximum supported value based on the
>> +     * buffer size allocated and the additional headroom required.
>> +     */
>> +    if (netdev == NULL || mtu <= 0

I don't really think that we need to check above things.
netdev can't be NULL here.  You may put an assert here if you worried.

mtu shuld be validated by db schema, and it can not be < 1.

>> +        || mtu > (FRAME_SIZE - OVS_XDP_HEADROOM - XDP_PACKET_HEADROOM)) {

mtu doesn't usually include ethernet header, vlans.  So, these should be
excluded too.  Not sure about FCS, if it passed to XDP program in generic
mode or it's stripped by the driver.

>> +        return EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  int
>>  netdev_afxdp_get_custom_stats(const struct netdev *netdev,
>>                                struct netdev_custom_stats *custom_stats)
>> diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h
>> index e2f400b72..ee6939fce 100644
>> --- a/lib/netdev-afxdp.h
>> +++ b/lib/netdev-afxdp.h
>> @@ -39,6 +39,7 @@ int netdev_afxdp_rxq_construct(struct netdev_rxq *rxq_);
>>  void netdev_afxdp_rxq_destruct(struct netdev_rxq *rxq_);
>>  int netdev_afxdp_construct(struct netdev *netdev_);
>>  void netdev_afxdp_destruct(struct netdev *netdev_);
>> +int netdev_afxdp_verify_mtu_size(const struct netdev *netdev, int mtu);
>>
>>  int netdev_afxdp_rxq_recv(struct netdev_rxq *rxq_,
>>                            struct dp_packet_batch *batch,
>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
>> index f48192373..89d0d9a9d 100644
>> --- a/lib/netdev-linux.c
>> +++ b/lib/netdev-linux.c
>> @@ -1593,6 +1593,15 @@ netdev_linux_set_mtu(struct netdev *netdev_, int mtu)
>>          goto exit;
>>      }
>>
>> +#ifdef HAVE_AF_XDP
>> +    if (!strcmp(netdev_get_type(netdev_), "afxdp")) {

It's better to compare netdev class with &netdev_afxdp_calss
as it done for tap.

>> +        error = netdev_afxdp_verify_mtu_size(netdev_, mtu);
>> +        if (error) {
>> +            goto exit;
>> +        }
>> +    }
>> +#endif
>> +
>>      if (netdev->cache_valid & VALID_MTU) {
>>          error = netdev->netdev_mtu_error;
>>          if (error || netdev->mtu == mtu) {
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>