* [PATCH] igc: fix invalid length and corrupted multi-segment mbufs
@ 2024-10-28 14:17 Martin Weiser
2024-10-29 17:42 ` Bruce Richardson
0 siblings, 1 reply; 5+ messages in thread
From: Martin Weiser @ 2024-10-28 14:17 UTC (permalink / raw)
To: dev
The issue only appeared with hardware-timestamping enabled
(RTE_ETH_RX_OFFLOAD_TIMESTAMP).
The length of the prepended hardware timestamp was not subtracted from
the data length so that received packets were 16 bytes longer than
expected.
In scatter-gather mode only the first mbuf has a timestamp but the
data offset of the follow-up mbufs was not adjusted accordingly.
This caused 16 bytes of packet data to be missing between
the segments.
Signed-off-by: Martin Weiser <martin.weiser@allegro-packets.com>
---
drivers/net/igc/igc_txrx.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
index d0cee1b016..2fafa91bd5 100644
--- a/drivers/net/igc/igc_txrx.c
+++ b/drivers/net/igc/igc_txrx.c
@@ -347,6 +347,8 @@ igc_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
rxm->data_off = RTE_PKTMBUF_HEADROOM;
data_len = rte_le_to_cpu_16(rxd.wb.upper.length) - rxq->crc_len;
+ if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
+ data_len -= IGC_TS_HDR_LEN;
rxm->data_len = data_len;
rxm->pkt_len = data_len;
rxm->nb_segs = 1;
@@ -509,6 +511,12 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
*/
rxm->data_off = RTE_PKTMBUF_HEADROOM;
data_len = rte_le_to_cpu_16(rxd.wb.upper.length);
+ if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
+ if (first_seg == NULL)
+ data_len -= IGC_TS_HDR_LEN;
+ else
+ rxm->data_off -= IGC_TS_HDR_LEN;
+ }
rxm->data_len = data_len;
/*
@@ -557,6 +565,7 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
last_seg->data_len = last_seg->data_len -
(RTE_ETHER_CRC_LEN - data_len);
last_seg->next = NULL;
+ rxm = last_seg;
} else {
rxm->data_len = (uint16_t)
(data_len - RTE_ETHER_CRC_LEN);
--
2.47.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] igc: fix invalid length and corrupted multi-segment mbufs
2024-10-28 14:17 [PATCH] igc: fix invalid length and corrupted multi-segment mbufs Martin Weiser
@ 2024-10-29 17:42 ` Bruce Richardson
2024-11-01 13:42 ` Martin Weiser
0 siblings, 1 reply; 5+ messages in thread
From: Bruce Richardson @ 2024-10-29 17:42 UTC (permalink / raw)
To: Martin Weiser; +Cc: dev
On Mon, Oct 28, 2024 at 03:17:07PM +0100, Martin Weiser wrote:
>
> The issue only appeared with hardware-timestamping enabled
> (RTE_ETH_RX_OFFLOAD_TIMESTAMP).
>
> The length of the prepended hardware timestamp was not subtracted from
> the data length so that received packets were 16 bytes longer than
> expected.
>
> In scatter-gather mode only the first mbuf has a timestamp but the
> data offset of the follow-up mbufs was not adjusted accordingly.
> This caused 16 bytes of packet data to be missing between
> the segments.
>
> Signed-off-by: Martin Weiser <martin.weiser@allegro-packets.com>
> ---
Hi,
thanks for the patch. Some comments inline below.
/Bruce
> drivers/net/igc/igc_txrx.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
> index d0cee1b016..2fafa91bd5 100644
> --- a/drivers/net/igc/igc_txrx.c
> +++ b/drivers/net/igc/igc_txrx.c
> @@ -347,6 +347,8 @@ igc_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
>
> rxm->data_off = RTE_PKTMBUF_HEADROOM;
> data_len = rte_le_to_cpu_16(rxd.wb.upper.length) - rxq->crc_len;
> + if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
> + data_len -= IGC_TS_HDR_LEN;
> rxm->data_len = data_len;
> rxm->pkt_len = data_len;
> rxm->nb_segs = 1;
> @@ -509,6 +511,12 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
> */
> rxm->data_off = RTE_PKTMBUF_HEADROOM;
> data_len = rte_le_to_cpu_16(rxd.wb.upper.length);
> + if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
> + if (first_seg == NULL)
> + data_len -= IGC_TS_HDR_LEN;
> + else
> + rxm->data_off -= IGC_TS_HDR_LEN;
This initially confused me, because I was assuming that data_off was a typo
for data_len. However, then I realised on closer examination that, when
timestamp offload is enabled, the actual buffer addresses sent down to the
hardware are offset by IGC_TS_HDR_LEN (meaning the first buffer of a pkt
has the start of data at "normal" data_offset in mbuf, but subsequent
buffers need adjustment). Is my understanding of the issue correct?
In either case, I have two small bits of feedback on this:
* Firstly, I think this needs a comment explaining the logic here, to avoid
others being confused as I was.
* Secondly, a very minor point, but is the code clearer or shorter, if you
merge this extra code down into the next block which is already checking
for the first_segment of a packet or not?
> + }
> rxm->data_len = data_len;
>
> /*
> @@ -557,6 +565,7 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
> last_seg->data_len = last_seg->data_len -
> (RTE_ETHER_CRC_LEN - data_len);
> last_seg->next = NULL;
> + rxm = last_seg;
> } else {
> rxm->data_len = (uint16_t)
> (data_len - RTE_ETHER_CRC_LEN);
> --
> 2.47.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] igc: fix invalid length and corrupted multi-segment mbufs
2024-10-29 17:42 ` Bruce Richardson
@ 2024-11-01 13:42 ` Martin Weiser
2024-11-01 13:52 ` Bruce Richardson
0 siblings, 1 reply; 5+ messages in thread
From: Martin Weiser @ 2024-11-01 13:42 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
Hi Bruce,
thank you very much for your feedback.
Please see my answers inline below.
I will send a v2 of the patch.
Best regards,
Martin
Am 29.10.24 um 18:42 schrieb Bruce Richardson:
> On Mon, Oct 28, 2024 at 03:17:07PM +0100, Martin Weiser wrote:
>>
>> The issue only appeared with hardware-timestamping enabled
>> (RTE_ETH_RX_OFFLOAD_TIMESTAMP).
>>
>> The length of the prepended hardware timestamp was not subtracted from
>> the data length so that received packets were 16 bytes longer than
>> expected.
>>
>> In scatter-gather mode only the first mbuf has a timestamp but the
>> data offset of the follow-up mbufs was not adjusted accordingly.
>> This caused 16 bytes of packet data to be missing between
>> the segments.
>>
>> Signed-off-by: Martin Weiser <martin.weiser@allegro-packets.com>
>> ---
>
> Hi,
>
> thanks for the patch. Some comments inline below.
>
> /Bruce
>
>> drivers/net/igc/igc_txrx.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
>> index d0cee1b016..2fafa91bd5 100644
>> --- a/drivers/net/igc/igc_txrx.c
>> +++ b/drivers/net/igc/igc_txrx.c
>> @@ -347,6 +347,8 @@ igc_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
>>
>> rxm->data_off = RTE_PKTMBUF_HEADROOM;
>> data_len = rte_le_to_cpu_16(rxd.wb.upper.length) - rxq->crc_len;
>> + if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
>> + data_len -= IGC_TS_HDR_LEN;
>> rxm->data_len = data_len;
>> rxm->pkt_len = data_len;
>> rxm->nb_segs = 1;
>> @@ -509,6 +511,12 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>> */
>> rxm->data_off = RTE_PKTMBUF_HEADROOM;
>> data_len = rte_le_to_cpu_16(rxd.wb.upper.length);
>> + if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
>> + if (first_seg == NULL)
>> + data_len -= IGC_TS_HDR_LEN;
>> + else
>> + rxm->data_off -= IGC_TS_HDR_LEN;
>
> This initially confused me, because I was assuming that data_off was a typo
> for data_len. However, then I realised on closer examination that, when
> timestamp offload is enabled, the actual buffer addresses sent down to the
> hardware are offset by IGC_TS_HDR_LEN (meaning the first buffer of a pkt
> has the start of data at "normal" data_offset in mbuf, but subsequent
> buffers need adjustment). Is my understanding of the issue correct?
This is exactly right. Took me a bit to understand what was happening here and
I do not know if there might be a better way do this than to change the start
offset of all descriptors. But there is probably a reason why it was done this way.
Initially the issue was detected as forwarding of packets that maxed out the MTU
failed since they now, with the increased length, exceeded the MTU. Only then it
became apparent that the scatter-gather handling was also broken.
>
> In either case, I have two small bits of feedback on this:
> * Firstly, I think this needs a comment explaining the logic here, to avoid
> others being confused as I was.
This will be part of patch v2.
> * Secondly, a very minor point, but is the code clearer or shorter, if you
> merge this extra code down into the next block which is already checking
> for the first_segment of a packet or not?
I would actually prefer to keep it that way as not to mix up the the data buffer
address and length handling with the mbuf chain construction.
>
>> + }
>> rxm->data_len = data_len;
>>
>> /*
>> @@ -557,6 +565,7 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>> last_seg->data_len = last_seg->data_len -
>> (RTE_ETHER_CRC_LEN - data_len);
>> last_seg->next = NULL;
>> + rxm = last_seg;
>> } else {
>> rxm->data_len = (uint16_t)
>> (data_len - RTE_ETHER_CRC_LEN);
>> --
>> 2.47.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] igc: fix invalid length and corrupted multi-segment mbufs
2024-11-01 13:42 ` Martin Weiser
@ 2024-11-01 13:52 ` Bruce Richardson
0 siblings, 0 replies; 5+ messages in thread
From: Bruce Richardson @ 2024-11-01 13:52 UTC (permalink / raw)
To: Martin Weiser; +Cc: dev
On Fri, Nov 01, 2024 at 02:42:20PM +0100, Martin Weiser wrote:
> Hi Bruce,
>
> thank you very much for your feedback.
> Please see my answers inline below.
>
> I will send a v2 of the patch.
>
> Best regards,
> Martin
>
>
> Am 29.10.24 um 18:42 schrieb Bruce Richardson:
> > On Mon, Oct 28, 2024 at 03:17:07PM +0100, Martin Weiser wrote:
> >>
> >> The issue only appeared with hardware-timestamping enabled
> >> (RTE_ETH_RX_OFFLOAD_TIMESTAMP).
> >>
> >> The length of the prepended hardware timestamp was not subtracted from
> >> the data length so that received packets were 16 bytes longer than
> >> expected.
> >>
> >> In scatter-gather mode only the first mbuf has a timestamp but the
> >> data offset of the follow-up mbufs was not adjusted accordingly.
> >> This caused 16 bytes of packet data to be missing between
> >> the segments.
> >>
> >> Signed-off-by: Martin Weiser <martin.weiser@allegro-packets.com>
> >> ---
> >
> > Hi,
> >
> > thanks for the patch. Some comments inline below.
> >
> > /Bruce
> >
> >> drivers/net/igc/igc_txrx.c | 9 +++++++++
> >> 1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
> >> index d0cee1b016..2fafa91bd5 100644
> >> --- a/drivers/net/igc/igc_txrx.c
> >> +++ b/drivers/net/igc/igc_txrx.c
> >> @@ -347,6 +347,8 @@ igc_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
> >>
> >> rxm->data_off = RTE_PKTMBUF_HEADROOM;
> >> data_len = rte_le_to_cpu_16(rxd.wb.upper.length) - rxq->crc_len;
> >> + if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
> >> + data_len -= IGC_TS_HDR_LEN;
> >> rxm->data_len = data_len;
> >> rxm->pkt_len = data_len;
> >> rxm->nb_segs = 1;
> >> @@ -509,6 +511,12 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
> >> */
> >> rxm->data_off = RTE_PKTMBUF_HEADROOM;
> >> data_len = rte_le_to_cpu_16(rxd.wb.upper.length);
> >> + if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
> >> + if (first_seg == NULL)
> >> + data_len -= IGC_TS_HDR_LEN;
> >> + else
> >> + rxm->data_off -= IGC_TS_HDR_LEN;
> >
> > This initially confused me, because I was assuming that data_off was a typo
> > for data_len. However, then I realised on closer examination that, when
> > timestamp offload is enabled, the actual buffer addresses sent down to the
> > hardware are offset by IGC_TS_HDR_LEN (meaning the first buffer of a pkt
> > has the start of data at "normal" data_offset in mbuf, but subsequent
> > buffers need adjustment). Is my understanding of the issue correct?
>
> This is exactly right. Took me a bit to understand what was happening here and
> I do not know if there might be a better way do this than to change the start
> offset of all descriptors. But there is probably a reason why it was done this way.
> Initially the issue was detected as forwarding of packets that maxed out the MTU
> failed since they now, with the increased length, exceeded the MTU. Only then it
> became apparent that the scatter-gather handling was also broken.
>
> >
> > In either case, I have two small bits of feedback on this:
> > * Firstly, I think this needs a comment explaining the logic here, to avoid
> > others being confused as I was.
>
> This will be part of patch v2.
>
> > * Secondly, a very minor point, but is the code clearer or shorter, if you
> > merge this extra code down into the next block which is already checking
> > for the first_segment of a packet or not?
>
> I would actually prefer to keep it that way as not to mix up the the data buffer
> address and length handling with the mbuf chain construction.
>
Ok, that is fine if you think it better. I'll just expect V2 with some
added comments to clarify things.
Thanks,
/Bruce
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] igc: fix invalid length and corrupted multi-segment mbufs
@ 2024-10-28 13:51 Martin Weiser
0 siblings, 0 replies; 5+ messages in thread
From: Martin Weiser @ 2024-10-28 13:51 UTC (permalink / raw)
To: dev
The issue only appeared with hardware-timestamping enabled
(RTE_ETH_RX_OFFLOAD_TIMESTAMP).
The length of the prepended hardware timestamp was not subtracted from
the data length so that received packets were 16 bytes longer than
expected.
In scatter-gather mode only the first mbuf has a timestamp but the
data offset of the follow-up mbufs was not adjusted accordingly.
This caused 16 bytes of packet data to be missing between
the segments.
Signed-off-by: Martin Weiser <martin.weiser@allegro-packets.com>
---
drivers/net/igc/igc_txrx.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
index d0cee1b016..2fafa91bd5 100644
--- a/drivers/net/igc/igc_txrx.c
+++ b/drivers/net/igc/igc_txrx.c
@@ -347,6 +347,8 @@ igc_recv_pkts(void *rx_queue, struct rte_mbuf
**rx_pkts, uint16_t nb_pkts)
rxm->data_off = RTE_PKTMBUF_HEADROOM;
data_len = rte_le_to_cpu_16(rxd.wb.upper.length) - rxq->crc_len;
+ if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
+ data_len -= IGC_TS_HDR_LEN;
rxm->data_len = data_len;
rxm->pkt_len = data_len;
rxm->nb_segs = 1;
@@ -509,6 +511,12 @@ igc_recv_scattered_pkts(void *rx_queue, struct
rte_mbuf **rx_pkts,
*/
rxm->data_off = RTE_PKTMBUF_HEADROOM;
data_len = rte_le_to_cpu_16(rxd.wb.upper.length);
+ if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
+ if (first_seg == NULL)
+ data_len -= IGC_TS_HDR_LEN;
+ else
+ rxm->data_off -= IGC_TS_HDR_LEN;
+ }
rxm->data_len = data_len;
/*
@@ -557,6 +565,7 @@ igc_recv_scattered_pkts(void *rx_queue, struct
rte_mbuf **rx_pkts,
last_seg->data_len = last_seg->data_len -
(RTE_ETHER_CRC_LEN - data_len);
last_seg->next = NULL;
+ rxm = last_seg;
} else {
rxm->data_len = (uint16_t)
(data_len - RTE_ETHER_CRC_LEN);
--
2.47.0
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-11-01 13:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-28 14:17 [PATCH] igc: fix invalid length and corrupted multi-segment mbufs Martin Weiser
2024-10-29 17:42 ` Bruce Richardson
2024-11-01 13:42 ` Martin Weiser
2024-11-01 13:52 ` Bruce Richardson
-- strict thread matches above, loose matches on Subject: below --
2024-10-28 13:51 Martin Weiser
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).