On Fri. 2 Apr 2021 at 19:22, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
