diff mbox series

[net] net: fec: Coverity issue: Dereference null return value

Message ID 20221215091149.936369-1-wei.fang@nxp.com
State New
Headers
Series [net] net: fec: Coverity issue: Dereference null return value |

Commit Message

Wei Fang Dec. 15, 2022, 9:11 a.m. UTC 
  From: Wei Fang <wei.fang@nxp.com>

The build_skb might return a null pointer but there is no check on the
return value in the fec_enet_rx_queue(). So a null pointer dereference
might occur. To avoid this, we check the return value of build_skb. If
the return value is a null pointer, the driver will recycle the page and
update the statistic of ndev. Then jump to rx_processing_done to clear
the status flags of the BD so that the hardware can recycle the BD.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
Reviewed-by: Shenwei Wang <Shenwei.wang@nxp.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Alexander Duyck Dec. 16, 2022, 3:34 p.m. UTC | #1 
On Thu, 2022-12-15 at 17:11 +0800, wei.fang@nxp.com wrote:
> From: Wei Fang <wei.fang@nxp.com>
> 
> The build_skb might return a null pointer but there is no check on the
> return value in the fec_enet_rx_queue(). So a null pointer dereference
> might occur. To avoid this, we check the return value of build_skb. If
> the return value is a null pointer, the driver will recycle the page and
> update the statistic of ndev. Then jump to rx_processing_done to clear
> the status flags of the BD so that the hardware can recycle the BD.
> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> Reviewed-by: Shenwei Wang <Shenwei.wang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 5528b0af82ae..c78aaa780983 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1674,6 +1674,16 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
>  		 * bridging applications.
>  		 */
>  		skb = build_skb(page_address(page), PAGE_SIZE);
> +		if (unlikely(!skb)) {
> +			page_pool_recycle_direct(rxq->page_pool, page);
> +			ndev->stats.rx_packets--;
> +			ndev->stats.rx_bytes -= pkt_len;
> +			ndev->stats.rx_dropped++;

I'm not sure you really need to bother with rewinding the rx_packets
and rx_bytes counters. I know that the rx_dropped statistic will get
incremented in the network stack in the event of a packet failing to
enqueue to the backlog, so it might be better to just leave the
rx_packets counter as is and assume the actual packet count is
rx_packets - rx_dropped.

> +
> +			netdev_err(ndev, "build_skb failed!\n");

Instead of netdev_err you may want to consider netdev_err_once for
this. Generally speaking when we start seeing memory allocation error
issues they can get very noisy very quickly as you are likely to fail
the allocation for every packet in a given polling session, and
sessions to follow.

> +			goto rx_processing_done;
> +		}
> +
>  		skb_reserve(skb, data_start);
>  		skb_put(skb, pkt_len - sub_len);
>  		skb_mark_for_recycle(skb);
Wei Fang Dec. 19, 2022, 2:21 a.m. UTC | #2 
> -----Original Message-----
> From: Alexander H Duyck <alexander.duyck@gmail.com>
> Sent: 2022年12月16日 23:34
> To: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Clark Wang
> <xiaoning.wang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>;
> dl-linux-imx <linux-imx@nxp.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net] net: fec: Coverity issue: Dereference null return value
> 
> On Thu, 2022-12-15 at 17:11 +0800, wei.fang@nxp.com wrote:
> > From: Wei Fang <wei.fang@nxp.com>
> >
> > The build_skb might return a null pointer but there is no check on the
> > return value in the fec_enet_rx_queue(). So a null pointer dereference
> > might occur. To avoid this, we check the return value of build_skb. If
> > the return value is a null pointer, the driver will recycle the page
> > and update the statistic of ndev. Then jump to rx_processing_done to
> > clear the status flags of the BD so that the hardware can recycle the BD.
> >
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > Reviewed-by: Shenwei Wang <Shenwei.wang@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index 5528b0af82ae..c78aaa780983 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -1674,6 +1674,16 @@ fec_enet_rx_queue(struct net_device *ndev, int
> budget, u16 queue_id)
> >  		 * bridging applications.
> >  		 */
> >  		skb = build_skb(page_address(page), PAGE_SIZE);
> > +		if (unlikely(!skb)) {
> > +			page_pool_recycle_direct(rxq->page_pool, page);
> > +			ndev->stats.rx_packets--;
> > +			ndev->stats.rx_bytes -= pkt_len;
> > +			ndev->stats.rx_dropped++;
> 
> I'm not sure you really need to bother with rewinding the rx_packets and
> rx_bytes counters. I know that the rx_dropped statistic will get incremented in
> the network stack in the event of a packet failing to enqueue to the backlog, so
> it might be better to just leave the rx_packets counter as is and assume the
> actual packet count is rx_packets - rx_dropped.
> 
According to your advice, I looked up the Linux document, actually as you said,
the rx_packets should include packets which host had to drop at various stages
of processing (even in the driver). Thanks for your review, I‘ll amend this in the
next version.

> > +
> > +			netdev_err(ndev, "build_skb failed!\n");
> 
> Instead of netdev_err you may want to consider netdev_err_once for this.
> Generally speaking when we start seeing memory allocation error issues they
> can get very noisy very quickly as you are likely to fail the allocation for every
> packet in a given polling session, and sessions to follow.
> 
Yes, it's better to use netdev_err_once than netdev_err in the situation you describe.
Thanks again!

> > +			goto rx_processing_done;
> > +		}
> > +
> >  		skb_reserve(skb, data_start);
> >  		skb_put(skb, pkt_len - sub_len);
> >  		skb_mark_for_recycle(skb);
Paolo Abeni Dec. 20, 2022, 2:46 p.m. UTC | #3 
Hello,

On Thu, 2022-12-15 at 17:11 +0800, wei.fang@nxp.com wrote:
> From: Wei Fang <wei.fang@nxp.com>
> 
> The build_skb might return a null pointer but there is no check on the
> return value in the fec_enet_rx_queue(). So a null pointer dereference
> might occur. To avoid this, we check the return value of build_skb. If
> the return value is a null pointer, the driver will recycle the page and
> update the statistic of ndev. Then jump to rx_processing_done to clear
> the status flags of the BD so that the hardware can recycle the BD.
> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> Reviewed-by: Shenwei Wang <Shenwei.wang@nxp.com>

You need to include a suitable fixes tag here. Please repost adding it
and retaining  Alex's reviwed-by tag, thanks!

Paolo
diff mbox series

Patch

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 5528b0af82ae..c78aaa780983 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1674,6 +1674,16 @@  fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
 		 * bridging applications.
 		 */
 		skb = build_skb(page_address(page), PAGE_SIZE);
+		if (unlikely(!skb)) {
+			page_pool_recycle_direct(rxq->page_pool, page);
+			ndev->stats.rx_packets--;
+			ndev->stats.rx_bytes -= pkt_len;
+			ndev->stats.rx_dropped++;
+
+			netdev_err(ndev, "build_skb failed!\n");
+			goto rx_processing_done;
+		}
+
 		skb_reserve(skb, data_start);
 		skb_put(skb, pkt_len - sub_len);
 		skb_mark_for_recycle(skb);