net/af_packet: tx_burst() can modify packets
https://bugs.dpdk.org/show_bug.cgi?id=1416
Bug ID: 1416
Summary: net/af_packet: tx_burst() can modify packets
Product: DPDK
Version: 24.03
Hardware: All
OS: All
Status: UNCONFIRMED
Severity: normal
Priority: Normal
Component: ethdev
Assignee: [email protected]
Reporter: [email protected]Target Milestone: ---
According to the ethdev doc, in general, PMD tx_burst() should not modify mbuf contents. To be more specific:
ethdev/rte_ethdev.h:6396
...
* @note This function must not modify mbufs (including packets data)
* unless the refcnt is 1.
* An exception is the bonding PMD, which does not have "Tx prepare" support,
* in this case, mbufs may be modified.
...
Though why looking at eth_af_packet_tx(), it looks to me like it does modify the packet contents without any checks for refcnt, etc.:
static uint16_teth_af_packet_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
{
...
for (i = 0; i < nb_pkts; i++) {
mbuf = *bufs++;
...
/* insert vlan info if necessary */
if (mbuf->ol_flags & RTE_MBUF_F_TX_VLAN) {
if (rte_vlan_insert(&mbuf)) {
rte_pktmbuf_free(mbuf);
continue; AFAIU, it does copy of mbuf contents into pbuf anyway (just few line below). So the fix might be - simply insert VLAN tag at copying stage. Feel free to correct me, if I missed something.
Signature · 2 lines
You are receiving this mail because: You are the assignee for the bug.
Quoted 15 lines · On Tue, 16 Apr 2024 10:29:53 +0000 [email protected] wrote:
static
uint16_teth_af_packet_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts){ ... for (i = 0; i < nb_pkts; i++) { mbuf = *bufs++; ... /* insert vlan info if necessary */ if (mbuf->ol_flags & RTE_MBUF_F_TX_VLAN) { if (rte_vlan_insert(&mbuf)) { rte_pktmbuf_free(mbuf); continue;
Quoted 3 lines
AFAIU, it does copy of mbuf contents into pbuf anyway (just few line below). So the fix might be - simply insert VLAN tag at copying stage. Feel free to correct me, if I missed something.
vlan_insert will fail if the mbuf is has refcnt > 1.
static inline int rte_vlan_insert(struct rte_mbuf **m)
{
struct rte_ether_hdr *oh, *nh;
struct rte_vlan_hdr *vh;
/* Can't insert header if mbuf is shared */
if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1)
return -EINVAL;Quoted 24 lines
Quoted 18 lines
static
uint16_teth_af_packet_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts){ ... for (i = 0; i < nb_pkts; i++) { mbuf = *bufs++; ... /* insert vlan info if necessary */ if (mbuf->ol_flags & RTE_MBUF_F_TX_VLAN) { if (rte_vlan_insert(&mbuf)) { rte_pktmbuf_free(mbuf); continue;Quoted 3 lines
AFAIU, it does copy of mbuf contents into pbuf anyway (just few line below). So the fix might be - simply insert VLAN tag at copying stage. Feel free to correct me, if I missed something.
vlan_insertwill fail if the mbuf is has refcnt > 1.
Quoted 8 lines
static inline int
rte_vlan_insert(struct rte_mbuf **m){ struct rte_ether_hdr *oh, *nh; struct rte_vlan_hdr *vh; /* Can't insert header if mbuf is shared */ if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1) return -EINVAL;
You are right, I missed that.
Will close it then.
Thanks
Konstantin
https://bugs.dpdk.org/show_bug.cgi?id=1416
Konstantin Ananyev ([email protected]) changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|UNCONFIRMED |RESOLVED
Resolution|--- |INVALID--- Comment #1 from Konstantin Ananyev ([email protected]) --- As Stephen Hemminger <[email protected]> pointed out:
vlan_insert will fail if the mbuf is has refcnt > 1.
static inline int rte_vlan_insert(struct rte_mbuf **m)
{
struct rte_ether_hdr *oh, *nh;
struct rte_vlan_hdr *vh;
/* Can't insert header if mbuf is shared */
if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1)
return -EINVAL;So closing as not a bug.
Signature · 2 lines
You are receiving this mail because: You are the assignee for the bug.
Quoted 28 lines
Quoted 2 lines
Quoted 2 lines
static
uint16_t
eth_af_packet_tx(void *queue, structrte_mbuf**bufs,uint16_t
nb_pkts)Quoted 7 lines
Quoted 7 lines
{ ... for (i = 0; i < nb_pkts; i++) { mbuf = *bufs++; ...Quoted 7 lines
Quoted 7 lines
/* insert vlan info if necessary */ if (mbuf->ol_flags & RTE_MBUF_F_TX_VLAN) { if (rte_vlan_insert(&mbuf)) { rte_pktmbuf_free(mbuf); continue;AFAIU, it does copy of mbuf contents into pbuf anyway (just few line
below).
Quoted 10 lines
Quoted 2 lines
So the fix might be - simply insert VLAN tag at copying stage. Feel free to correct me, if I missed something.
vlan_insertwill fail if the mbuf is has refcnt > 1.static inline int
rte_vlan_insert(struct rte_mbuf **m){ struct rte_ether_hdr *oh, *nh; struct rte_vlan_hdr *vh;
Quoted 6 lines
Quoted 3 lines
/* Can't insert header if mbuf is shared */ if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1) return -EINVAL;You are right, I missed that.
Will close it then.
Don't close, silent drop is also a bug.
The VLAN tag could be insert when copying, as originally suggested.
Alternatively, set tpacket2_hdr's tp_vlan_tci and tp_vlan_tpid fields and set TP_STATUS_VLAN_VALID and TP_STATUS_VLAN_TPID_VALID flags in tp_status. If the kernel reads those?
Quoted 34 lines
Quoted 34 lines
Quoted 4 lines
Quoted 4 lines
static
uint16_t
eth_af_packet_tx(void *queue, structrte_mbuf**bufs,uint16_t
nb_pkts)Quoted 7 lines
Quoted 7 lines
{ ... for (i = 0; i < nb_pkts; i++) { mbuf = *bufs++; ...Quoted 7 lines
Quoted 7 lines
/* insert vlan info if necessary */ if (mbuf->ol_flags & RTE_MBUF_F_TX_VLAN) { if (rte_vlan_insert(&mbuf)) { rte_pktmbuf_free(mbuf); continue;AFAIU, it does copy of mbuf contents into pbuf anyway (just few line
below).
Quoted 13 lines
Quoted 2 lines
So the fix might be - simply insert VLAN tag at copying stage. Feel free to correct me, if I missed something.
vlan_insertwill fail if the mbuf is has refcnt > 1.static inline int
rte_vlan_insert(struct rte_mbuf **m){ struct rte_ether_hdr *oh, *nh; struct rte_vlan_hdr *vh; /* Can't insert header if mbuf is shared */ if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1) return -EINVAL;
Quoted 6 lines
Quoted 2 lines
You are right, I missed that.
Will close it then.Don't close, silent drop is also a bug.
The VLAN tag could be insert when copying, as originally suggested.
Agree, but to me that would be enhancement request, not a bug report.
Quoted 3 lines
Alternatively, set tpacket2_hdr's
tp_vlan_tciandtp_vlan_tpidfields and setTP_STATUS_VLAN_VALIDandTP_STATUS_VLAN_TPID_VALIDflags intp_status. If the kernel reads those?
Quoted 33 lines
Quoted 10 lines
Quoted 10 lines
Quoted 2 lines
Quoted 2 lines
static
uint16_t
eth_af_packet_tx(void *queue, structrte_mbuf**bufs,uint16_t
nb_pkts)Quoted 7 lines
Quoted 7 lines
{ ... for (i = 0; i < nb_pkts; i++) { mbuf = *bufs++; ...Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
/* insert vlan info if necessary */ if (mbuf->ol_flags & RTE_MBUF_F_TX_VLAN) { if (rte_vlan_insert(&mbuf)) { rte_pktmbuf_free(mbuf); continue;AFAIU, it does copy of mbuf contents into pbuf anyway (just few
line
Quoted 15 lines
Quoted 15 lines
below).
Quoted 13 lines
Quoted 2 lines
So the fix might be - simply insert VLAN tag at copying stage. Feel free to correct me, if I missed something.
vlan_insertwill fail if the mbuf is has refcnt > 1.static inline int
rte_vlan_insert(struct rte_mbuf **m){ struct rte_ether_hdr *oh, *nh; struct rte_vlan_hdr *vh; /* Can't insert header if mbuf is shared */ if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1) return -EINVAL;
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
You are right, I missed that.
Will close it then.Don't close, silent drop is also a bug.
The VLAN tag could be insert when copying, as originally suggested.
Agree, but to me that would be enhancement request, not a bug report.
Hmm... there is still a bug, although slightly different:
net/af_packet: tx_burst() silently drops packets with RTE_MBUF_F_TX_VLAN if mbuf is shared
And the suggested fixes would fix this (other) bug.
Quoted 6 lines
Quoted 3 lines
Alternatively, set tpacket2_hdr's
tp_vlan_tciandtp_vlan_tpidfieldsand set
TP_STATUS_VLAN_VALIDandQuoted 1 line
TP_STATUS_VLAN_TPID_VALIDflags intp_status. If the kernel readsthose?
Stephen Hemminger
Acked-by Olivier MatzQuoted 40 lines · On Tue, 16 Apr 2024 20:11:05 +0200 Morten Brørup <[email protected]>…
Quoted 40 lines
Quoted 10 lines
Quoted 10 lines
Quoted 2 lines
Quoted 2 lines
static
uint16_t
eth_af_packet_tx(void *queue, structrte_mbuf**bufs,uint16_t
nb_pkts)Quoted 7 lines
Quoted 7 lines
{ ... for (i = 0; i < nb_pkts; i++) { mbuf = *bufs++; ...Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
/* insert vlan info if necessary */ if (mbuf->ol_flags & RTE_MBUF_F_TX_VLAN) { if (rte_vlan_insert(&mbuf)) { rte_pktmbuf_free(mbuf); continue;AFAIU, it does copy of mbuf contents into pbuf anyway (just few
line
Quoted 21 lines
Quoted 11 lines
below).
Quoted 10 lines
Quoted 2 lines
So the fix might be - simply insert VLAN tag at copying stage. Feel free to correct me, if I missed something.
vlan_insertwill fail if the mbuf is has refcnt > 1.static inline int
rte_vlan_insert(struct rte_mbuf **m){ struct rte_ether_hdr *oh, *nh; struct rte_vlan_hdr *vh;Quoted 6 lines
Quoted 3 lines
/* Can't insert header if mbuf is shared */ if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1) return -EINVAL;You are right, I missed that.
Will close it then.Don't close, silent drop is also a bug.
The VLAN tag could be insert when copying, as originally suggested.
Quoted 7 lines
Quoted 1 line
Agree, but to me that would be enhancement request, not a bug report.
Hmm... there is still a bug, although slightly different:
net/af_packet:tx_burst()silently drops packets withRTE_MBUF_F_TX_VLANif mbuf is sharedAnd the suggested fixes would fix this (other) bug.
In older DPDK, vlan_insert would try and clone the mbuf, but doing a rte_pktmbuf_clone(). But that was buggy and removed by:
commit 15a74163b12ed9b8b980b1576bdd8de16d60612b
Author: Ferruh Yigit <[email protected]> Date: Tue Apr 16 16:51:26 2019 +0100
net: forbid VLAN insert in shared mbuf
The vlan_insert() is buggy when it tries to handle the shared mbufs, instead don't support inserting VLAN tag into shared mbufs and return an error for that case.
+TO: John W. Linville, AF_PACKET maintainer
Quoted 44 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
static
uint16_t
eth_af_packet_tx(void *queue, structrte_mbuf**bufs,
uint16_tQuoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
nb_pkts)Quoted 7 lines
Quoted 7 lines
{ ... for (i = 0; i < nb_pkts; i++) { mbuf = *bufs++; ...Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
/* insert vlan info if necessary */ if (mbuf->ol_flags & RTE_MBUF_F_TX_VLAN) { if (rte_vlan_insert(&mbuf)) { rte_pktmbuf_free(mbuf); continue;AFAIU, it does copy of mbuf contents into pbuf anyway (just
few
Quoted 3 lines
Quoted 3 lines
line
Quoted 2 lines
Quoted 2 lines
below).
Quoted 1 line
Quoted 1 line
So the fix might be - simply insert VLAN tag at copying
stage.
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
Quoted 1 line
Feel free to correct me, if I missed something.
vlan_insertwill fail if the mbuf is has refcnt > 1.static inline int
rte_vlan_insert(struct rte_mbuf **m){ struct rte_ether_hdr *oh, *nh; struct rte_vlan_hdr *vh; /* Can't insert header if mbuf is shared */ if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1) return -EINVAL;Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
You are right, I missed that.
Will close it then.Don't close, silent drop is also a bug.
The VLAN tag could be insert when copying, as originally
suggested.
Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 1 line
Quoted 1 line
Agree, but to me that would be enhancement request, not a bug
report.
Quoted 4 lines
Hmm... there is still a bug, although slightly different:
net/af_packet:tx_burst()silently drops packets with
RTE_MBUF_F_TX_VLANif mbuf is shared
I took a more thorough look at the code.
I was wrong: The drop is not silent, the err_pkts counter is incremented.
But still a bug (non-conformance) to claim VLAN Insert capability, and not fully support it.
Quoted 2 lines
Quoted 2 lines
And the suggested fixes would fix this (other) bug.
The kernel doesn't look at TP_STATUS_VLAN_VALID in TX direction, so setting the VLAN tag in the tpacket2_hdr doesn't work; inserting it in the data when copying would be the fix.
Before the copy loop, the first segment should be copied, possibly with VLAN insertion. And the copy loop should copy the following segments (if any), i.e. starting at *tmp_mbuf = mbuf->next.
As a separate bug, the check for oversize packets doesn't consider the length of an inserted VLAN tag, and could cause a buffer overrun if the packet is large enough. However, with a default buffer size of 2048, this is very unlikely in reality.
Quoted 21 lines
In older DPDK,
vlan_insertwould try and clone the mbuf, but doing arte_pktmbuf_clone().
But that was buggy and removed by:commit
15a74163b12ed9b8b980b1576bdd8de16d60612bAuthor: Ferruh Yigit <[email protected]> Date: Tue Apr 16 16:51:26 2019 +0100net: forbid VLAN insert in shared mbuf
The
vlan_insert()is buggy when it tries to handle the shared mbufs, instead don't support inserting VLAN tag into shared mbufs and
return
an error for that case.Signed-off-by Ferruh Yigit <[email protected]>Acked-by Olivier Matz <[email protected]>