[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