Re: [PATCH] can: skb: alloc_can{,fd}_skb(): set "cf" to NULL if skb allocation fails

From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
To: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: linux-can <linux-can@vger.kernel.org>
Subject: Re: [PATCH] can: skb: alloc_can{,fd}_skb(): set "cf" to NULL if skb allocation fails
Date: Fri, 2 Apr 2021 21:42:20 +0900	[thread overview]
Message-ID: <CAMZ6RqKYKcmcx0PFo4CsNh3d1bCCr8f=G8zLxt9gQy42E3AR2w@mail.gmail.com> (raw)
In-Reply-To: <20210402102245.1512583-1-mkl@pengutronix.de>

On Fri. 2 Apr 2021 at 19:22, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> The handling of CAN bus errors typically consist of allocating a CAN
> error SKB using alloc_can_err_skb() followed by stats handling and
> filling the error details in the newly allocated CAN error SKB. Even
> if the allocation of the SKB fails the stats handling should not be
> skipped.
>
> The common pattern in CAN drivers is to allocate the skb and work on
> the struct can_frame pointer "cf", if it has been assigned by
> alloc_can_err_skb().
>
> |       skb = alloc_can_err_skb(priv->ndev, &cf);
> |
> |       /* RX errors */
> |       if (bdiag1 & (MCP251XFD_REG_BDIAG1_DCRCERR |
> |                     MCP251XFD_REG_BDIAG1_NCRCERR)) {
> |               netdev_dbg(priv->ndev, "CRC error\n");
> |
> |               stats->rx_errors++;
> |               if (cf)
> |                       cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
> |       }
>
> In case of an OOM alloc_can_err_skb() returns NULL, but doesn't set
> "cf" to NULL as well. For the above pattern to work the "cf" has to be
> initialized to NULL, which is easily forgotten.
>
> To solve this kind of problems, set "cf" to NULL if
> alloc_can_err_skb() returns NULL.
>
> Suggested-by: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/dev/skb.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 387c0bc0fb9c..61660248c69e 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -183,8 +183,11 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
>
>         skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
>                                sizeof(struct can_frame));
> -       if (unlikely(!skb))
> +       if (unlikely(!skb)) {
> +               *cf = NULL;
> +
>                 return NULL;
> +       }
>
>         skb->protocol = htons(ETH_P_CAN);
>         skb->pkt_type = PACKET_BROADCAST;
> @@ -211,8 +214,11 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
>
>         skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
>                                sizeof(struct canfd_frame));
> -       if (unlikely(!skb))
> +       if (unlikely(!skb)) {
> +               *cfd = NULL;
> +
>                 return NULL;
> +       }
>
>         skb->protocol = htons(ETH_P_CANFD);
>         skb->pkt_type = PACKET_BROADCAST;

Thanks Marc!

Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>