Re: [ovs-dev] [PATCH] netdev-afxdp: add afxdp specific maximum MTU check
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
---
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(>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 _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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev-afxdp: add afxdp specific maximum MTU check
On 8 Nov 2019, at 2:34, William Tu wrote:
On Thu, Nov 07, 2019 at 03:01:18PM +0100, 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, 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
---
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(>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
+|| mtu > (FRAME_SIZE - OVS_XDP_HEADROOM -
XDP_PACKET_HEADROOM)) {
I remember XDP max MTU = 3520 bytes,
and it's (page_size(4096) - headroom(256) - shinfo(320))
so here we should also subtract shinfo?
That is the theoretical maximum, however you’re code allocated chunks
of FRAME_SIZE, so that dictated the maximum value.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev-afxdp: add afxdp specific maximum MTU check
On Thu, Nov 07, 2019 at 03:01:18PM +0100, 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, 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
> >---
> > 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(>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
> >+|| mtu > (FRAME_SIZE - OVS_XDP_HEADROOM - XDP_PACKET_HEADROOM)) {
I remember XDP max MTU = 3520 bytes,
and it's (page_size(4096) - headroom(256) - shinfo(320))
so here we should also subtract shinfo?
Regards,
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev-afxdp: add afxdp specific maximum MTU check
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
---
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(>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 _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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev-afxdp: add afxdp specific maximum MTU check
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, 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
---
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(>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
+|| mtu > (FRAME_SIZE - OVS_XDP_HEADROOM -
XDP_PACKET_HEADROOM)) {
+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")) {
+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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] netdev-afxdp: add afxdp specific maximum MTU check
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, 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
---
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(>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
+|| mtu > (FRAME_SIZE - OVS_XDP_HEADROOM - XDP_PACKET_HEADROOM)) {
+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")) {
+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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
