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

Eelco Chaudron echaudro at redhat.com
Fri Nov 8 08:55:16 UTC 2019



On 7 Nov 2019, at 17:43, Ilya Maximets wrote:

> 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?

Cant remember the details but I did see an easy fix so decided to fix it 
here first :)

>>> 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.

Ok will remove it…

>>> +        || 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.

Thought I did some tests and the MTU given was from the ethernet header 
till the FCS (not included).
But I’ll re-test it on the new revision, and update the code here if 
needed…

>>> +        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.

Will do this in next version!

>>> +        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
>>



More information about the dev mailing list