| Message ID | 20250217093906.506214-2-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: enetc: fix some known issues | expand |
On Mon, Feb 17, 2025 at 05:38:59PM +0800, Wei Fang wrote: > When the DMA mapping error occurs, it will attempt to free 'count + 1' > tx_swbd instead of 'count', so fix this off-by-one issue. > > Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers") > Cc: stable@vger.kernel.org > Signed-off-by: Wei Fang <wei.fang@nxp.com> > --- > drivers/net/ethernet/freescale/enetc/enetc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index 6a6fc819dfde..f7bc2fc33a76 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -372,13 +372,13 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb) > dma_err: > dev_err(tx_ring->dev, "DMA map error"); > > - do { > + while (count--) { > tx_swbd = &tx_ring->tx_swbd[i]; > enetc_free_tx_frame(tx_ring, tx_swbd); > if (i == 0) > i = tx_ring->bd_count; > i--; > - } while (count--); > + }; In enetc_lso_hw_offload() this is fixed by --count instead of changing to while and count--, maybe follow this scheme, or event better call helper function to fix in one place. The same problem is probably in enetc_map_tx_tso_buffs(). Thanks Michal > > return 0; > } > -- > 2.34.1 >
On Mon, 17 Feb 2025 17:38:59 +0800 Wei Fang wrote: > + while (count--) { > tx_swbd = &tx_ring->tx_swbd[i]; > enetc_free_tx_frame(tx_ring, tx_swbd); > if (i == 0) > i = tx_ring->bd_count; > i--; > - } while (count--); > + }; I think this gives us: drivers/net/ethernet/freescale/enetc/enetc.c:408:2-3: Unneeded semicolon
> -----Original Message----- > From: Wei Fang <wei.fang@nxp.com> > Sent: Monday, February 17, 2025 11:39 AM [...] > Subject: [PATCH net 1/8] net: enetc: fix the off-by-one issue in > enetc_map_tx_buffs() > > When the DMA mapping error occurs, it will attempt to free 'count + 1' Hi Wei, Are you sure? dma_err occurs before count is incremented, at least that's the design. First step already contradicts your claim: ``` static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb) { [...] int i, count = 0; [...] dma = dma_map_single(tx_ring->dev, skb->data, len, DMA_TO_DEVICE); if (unlikely(dma_mapping_error(tx_ring->dev, dma))) goto dma_err; ===> count is 0 on goto path! [...] count++; ``` Regards, Claudiu
> > -----Original Message----- > > From: Wei Fang <wei.fang@nxp.com> > > Sent: Monday, February 17, 2025 11:39 AM > [...] > > Subject: [PATCH net 1/8] net: enetc: fix the off-by-one issue in > > enetc_map_tx_buffs() > > > > When the DMA mapping error occurs, it will attempt to free 'count + 1' > > Hi Wei, > Are you sure? > > dma_err occurs before count is incremented, at least that's the design. > > First step already contradicts your claim: > ``` > static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb) > { [...] > int i, count = 0; > [...] > dma = dma_map_single(tx_ring->dev, skb->data, len, DMA_TO_DEVICE); > if (unlikely(dma_mapping_error(tx_ring->dev, dma))) > goto dma_err; > > ===> count is 0 on goto path! > [...] > count++; > ``` > Hi Claudiu, I think it's fine in your case, the count is 0, and the tx_swbd is not set, so it's unnecessary to call enetc_free_tx_frame() in the dma_err path. But if the count is not zero, then that is the problem, for example, count is 1, and then goto dma_err path, the it will attempt to free 2 tx_swbd.
> > Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet > drivers") > > Cc: stable@vger.kernel.org > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > --- > > drivers/net/ethernet/freescale/enetc/enetc.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > b/drivers/net/ethernet/freescale/enetc/enetc.c > > index 6a6fc819dfde..f7bc2fc33a76 100644 > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > @@ -372,13 +372,13 @@ static int enetc_map_tx_buffs(struct enetc_bdr > *tx_ring, struct sk_buff *skb) > > dma_err: > > dev_err(tx_ring->dev, "DMA map error"); > > > > - do { > > + while (count--) { > > tx_swbd = &tx_ring->tx_swbd[i]; > > enetc_free_tx_frame(tx_ring, tx_swbd); > > if (i == 0) > > i = tx_ring->bd_count; > > i--; > > - } while (count--); > > + }; > > In enetc_lso_hw_offload() this is fixed by --count instead of changing > to while and count--, maybe follow this scheme, or event better call > helper function to fix in one place. The situation is slightly different in enetc_map_tx_buffs(), the count may be 0 when the error occurs. But in enetc_lso_hw_offload(), the count will not be 0 when the error occurs. > > The same problem is probably in enetc_map_tx_tso_buffs(). > I think there is no such problem in enetc_map_tx_tso_buffs(), because the index 'i' has been increased before the error occurs, but the count is not increased, so the actual 'count' is count + 1.
> On Mon, 17 Feb 2025 17:38:59 +0800 Wei Fang wrote: > > + while (count--) { > > tx_swbd = &tx_ring->tx_swbd[i]; > > enetc_free_tx_frame(tx_ring, tx_swbd); > > if (i == 0) > > i = tx_ring->bd_count; > > i--; > > - } while (count--); > > + }; > > I think this gives us: > > drivers/net/ethernet/freescale/enetc/enetc.c:408:2-3: Unneeded semicolon Thanks, will fix it. > -- > pw-bot: cr
On Tue, Feb 18, 2025 at 02:11:12AM +0000, Wei Fang wrote: > > > Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet > > drivers") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > > --- > > > drivers/net/ethernet/freescale/enetc/enetc.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > > b/drivers/net/ethernet/freescale/enetc/enetc.c > > > index 6a6fc819dfde..f7bc2fc33a76 100644 > > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > > @@ -372,13 +372,13 @@ static int enetc_map_tx_buffs(struct enetc_bdr > > *tx_ring, struct sk_buff *skb) > > > dma_err: > > > dev_err(tx_ring->dev, "DMA map error"); > > > > > > - do { > > > + while (count--) { > > > tx_swbd = &tx_ring->tx_swbd[i]; > > > enetc_free_tx_frame(tx_ring, tx_swbd); > > > if (i == 0) > > > i = tx_ring->bd_count; > > > i--; > > > - } while (count--); > > > + }; > > > > In enetc_lso_hw_offload() this is fixed by --count instead of changing > > to while and count--, maybe follow this scheme, or event better call > > helper function to fix in one place. > > The situation is slightly different in enetc_map_tx_buffs(), the count > may be 0 when the error occurs. But in enetc_lso_hw_offload(), the > count will not be 0 when the error occurs. Right, didn't see that, sorry. > > > > > The same problem is probably in enetc_map_tx_tso_buffs(). > > > > I think there is no such problem in enetc_map_tx_tso_buffs(), > because the index 'i' has been increased before the error occurs, > but the count is not increased, so the actual 'count' is count + 1. > But you can reach the error code jumping to label "err_chained_bd" where both i and count is increased. Isn't it a problem? BTW, not increasing i and count in one place looks like unnecessary complication ;) . Thanks, Michal
> -----Original Message----- > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Sent: 2025年2月18日 13:45 > To: Wei Fang <wei.fang@nxp.com> > Cc: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>; Claudiu Manoil > <claudiu.manoil@nxp.com>; Vladimir Oltean <vladimir.oltean@nxp.com>; > Clark Wang <xiaoning.wang@nxp.com>; andrew+netdev@lunn.ch; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; Ioana Ciornei <ioana.ciornei@nxp.com>; Y.B. Lu > <yangbo.lu@nxp.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > imx@lists.linux.dev; stable@vger.kernel.org > Subject: Re: [PATCH net 1/8] net: enetc: fix the off-by-one issue in > enetc_map_tx_buffs() > > On Tue, Feb 18, 2025 at 02:11:12AM +0000, Wei Fang wrote: > > > > Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet > > > drivers") > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > > > --- > > > > drivers/net/ethernet/freescale/enetc/enetc.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > > > b/drivers/net/ethernet/freescale/enetc/enetc.c > > > > index 6a6fc819dfde..f7bc2fc33a76 100644 > > > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > > > @@ -372,13 +372,13 @@ static int enetc_map_tx_buffs(struct > enetc_bdr > > > *tx_ring, struct sk_buff *skb) > > > > dma_err: > > > > dev_err(tx_ring->dev, "DMA map error"); > > > > > > > > - do { > > > > + while (count--) { > > > > tx_swbd = &tx_ring->tx_swbd[i]; > > > > enetc_free_tx_frame(tx_ring, tx_swbd); > > > > if (i == 0) > > > > i = tx_ring->bd_count; > > > > i--; > > > > - } while (count--); > > > > + }; > > > > > > In enetc_lso_hw_offload() this is fixed by --count instead of changing > > > to while and count--, maybe follow this scheme, or event better call > > > helper function to fix in one place. > > > > The situation is slightly different in enetc_map_tx_buffs(), the count > > may be 0 when the error occurs. But in enetc_lso_hw_offload(), the > > count will not be 0 when the error occurs. > > Right, didn't see that, sorry. > > > > > > > > > The same problem is probably in enetc_map_tx_tso_buffs(). > > > > > > > I think there is no such problem in enetc_map_tx_tso_buffs(), > > because the index 'i' has been increased before the error occurs, > > but the count is not increased, so the actual 'count' is count + 1. > > > > But you can reach the error code jumping to label "err_chained_bd" where > both i and count is increased. Isn't it a problem? > You are right, I ignored this label. I will add a separate patch to fix this issue. Thanks. > BTW, not increasing i and count in one place looks like unnecessary > complication ;) . >
> -----Original Message----- > From: Wei Fang <wei.fang@nxp.com> > Sent: Tuesday, February 18, 2025 4:03 AM [,,,] > Subject: RE: [PATCH net 1/8] net: enetc: fix the off-by-one issue in > enetc_map_tx_buffs() > > > > -----Original Message----- > > > From: Wei Fang <wei.fang@nxp.com> > > > Sent: Monday, February 17, 2025 11:39 AM > > [...] > > > Subject: [PATCH net 1/8] net: enetc: fix the off-by-one issue in > > > enetc_map_tx_buffs() > > > > > > When the DMA mapping error occurs, it will attempt to free 'count + 1' > > > > Hi Wei, > > Are you sure? > > > > dma_err occurs before count is incremented, at least that's the design. > > > > First step already contradicts your claim: > > ``` > > static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb) > > { [...] > > int i, count = 0; > > [...] > > dma = dma_map_single(tx_ring->dev, skb->data, len, > DMA_TO_DEVICE); > > if (unlikely(dma_mapping_error(tx_ring->dev, dma))) > > goto dma_err; > > > > ===> count is 0 on goto path! > > [...] > > count++; > > ``` > > > > Hi Claudiu, > > I think it's fine in your case, the count is 0, and the tx_swbd is not set, > so it's unnecessary to call enetc_free_tx_frame() in the dma_err path. > enetc_free_tx_frame() call on dma_err path is still needed for count 0 because it needs to free the skb.
> > -----Original Message----- > > From: Wei Fang <wei.fang@nxp.com> > > Sent: Tuesday, February 18, 2025 4:03 AM > [,,,] > > Subject: RE: [PATCH net 1/8] net: enetc: fix the off-by-one issue in > > enetc_map_tx_buffs() > > > > > > -----Original Message----- > > > > From: Wei Fang <wei.fang@nxp.com> > > > > Sent: Monday, February 17, 2025 11:39 AM > > > [...] > > > > Subject: [PATCH net 1/8] net: enetc: fix the off-by-one issue in > > > > enetc_map_tx_buffs() > > > > > > > > When the DMA mapping error occurs, it will attempt to free 'count + 1' > > > > > > Hi Wei, > > > Are you sure? > > > > > > dma_err occurs before count is incremented, at least that's the design. > > > > > > First step already contradicts your claim: > > > ``` > > > static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff > *skb) > > > { [...] > > > int i, count = 0; > > > [...] > > > dma = dma_map_single(tx_ring->dev, skb->data, len, > > DMA_TO_DEVICE); > > > if (unlikely(dma_mapping_error(tx_ring->dev, dma))) > > > goto dma_err; > > > > > > ===> count is 0 on goto path! > > > [...] > > > count++; > > > ``` > > > > > > > Hi Claudiu, > > > > I think it's fine in your case, the count is 0, and the tx_swbd is not set, > > so it's unnecessary to call enetc_free_tx_frame() in the dma_err path. > > > > enetc_free_tx_frame() call on dma_err path is still needed for count 0 because > it needs to free the skb. First, the tx_swbd is not set when count is 0, so tx_swbd->skb is NULL when the error occurs. Second, the skb is freed in enetc_start_xmit() not enetc_free_tx_frame().
> -----Original Message----- > From: Wei Fang <wei.fang@nxp.com> > Sent: Tuesday, February 18, 2025 11:18 AM [...] > > enetc_free_tx_frame() call on dma_err path is still needed for count 0 > > because it needs to free the skb. > > First, the tx_swbd is not set when count is 0, so tx_swbd->skb is NULL when > the error occurs. > > Second, the skb is freed in enetc_start_xmit() not enetc_free_tx_frame(). Yeah, noticed too finally. 'do {} while()' is not ok on error path here.
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 6a6fc819dfde..f7bc2fc33a76 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -372,13 +372,13 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb) dma_err: dev_err(tx_ring->dev, "DMA map error"); - do { + while (count--) { tx_swbd = &tx_ring->tx_swbd[i]; enetc_free_tx_frame(tx_ring, tx_swbd); if (i == 0) i = tx_ring->bd_count; i--; - } while (count--); + }; return 0; }
When the DMA mapping error occurs, it will attempt to free 'count + 1' tx_swbd instead of 'count', so fix this off-by-one issue. Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers") Cc: stable@vger.kernel.org Signed-off-by: Wei Fang <wei.fang@nxp.com> --- drivers/net/ethernet/freescale/enetc/enetc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)