Re: [PATCH] crypto: chtls: Add check alloc_skb() returned value

[Simon Horman <horms@kernel.org>]

On Fri, Jan 17, 2025 at 11:13:28AM +0800, Charles Han wrote:
> alloc_skb() can return a NULL pointer on failure.But these returned
> value in send_defer_abort_rpl() and chtls_close_conn()  not checked.
> 
> Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
> Signed-off-by: Charles Han <hanchunchao@inspur.com>
> ---
>  .../net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c  | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> index 6f6525983130..725cce34f25a 100644
> --- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> +++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> @@ -306,6 +306,10 @@ static void chtls_close_conn(struct sock *sk)
>  	tid = csk->tid;
>  
>  	skb = alloc_skb(len, GFP_KERNEL | __GFP_NOFAIL);
> +	if (!skb) {
> +		pr_warn("%s: cannot allocate skb!\n", __func__);
> +		return;
> +	}
>  	req = (struct cpl_close_con_req *)__skb_put(skb, len);
>  	memset(req, 0, len);
>  	req->wr.wr_hi = htonl(FW_WR_OP_V(FW_TP_WR) |
> @@ -1991,6 +1995,11 @@ static void send_defer_abort_rpl(struct chtls_dev *cdev, struct sk_buff *skb)
>  
>  	reply_skb = alloc_skb(sizeof(struct cpl_abort_rpl),
>  			      GFP_KERNEL | __GFP_NOFAIL);
> +	if (!reply_skb) {
> +		pr_warn("%s: cannot allocate skb!\n", __func__);
> +		return;
> +	}
> +
>  	__skb_put(reply_skb, sizeof(struct cpl_abort_rpl));
>  	set_abort_rpl_wr(reply_skb, GET_TID(req),
>  			 (req->status & CPL_ABORT_NO_RST));

Hi Charles,

I agree that not checking for NULL skbs will very soon lead
to a NULL pointer dereference. But I wonder if this patch leads
us to a better place. Because by returning on skb allocation
failure in each of the above cases, don't we end up with
an inconsistent state?

Also, the above notwithstanding, I do wonder if:
a) the warnings should be errors
b) they should be rate limited