netdev - Re: [net v2 3/7] cxgb4/ch_ktls: creating skbs causes panic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201023190503.44f61271@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date:   Fri, 23 Oct 2020 19:05:03 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Rohit Maheshwari <rohitm@...lsio.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, secdev@...lsio.com,
        Julia Lawall <julia.lawall@...ia.fr>
Subject: Re: [net v2 3/7] cxgb4/ch_ktls: creating skbs causes panic

On Fri, 23 Oct 2020 11:01:30 +0530 Rohit Maheshwari wrote:
> Creating SKB per tls record and freeing the original one causes
> panic. There will be race if connection reset is requested. By
> freeing original skb, refcnt will be decremented and that means,
> there is no pending record to send, and so tls_dev_del will be
> requested in control path while SKB of related connection is in
> queue.

Can't you make your new skbs take a reference on the socket, then?

> @@ -1672,42 +1572,60 @@ static int chcr_end_part_handler(struct chcr_ktls_info *tx_info,
>  				 struct tls_record_info *record,
>  				 u32 tcp_seq, int mss, bool tcp_push_no_fin,
>  				 struct sge_eth_txq *q,
> -				 u32 tls_end_offset, bool last_wr)
> +				 u32 tls_end_offset, u32 skb_offset)
>  {
>  	struct sk_buff *nskb = NULL;
> +	bool is_last_wr = false;
> +	int ret;
> +
> +	if (skb_offset + tls_end_offset == skb->len)
> +		is_last_wr = true;
> +
>  	/* check if it is a complete record */
>  	if (tls_end_offset == record->len) {
>  		nskb = skb;
>  		atomic64_inc(&tx_info->adap->ch_ktls_stats.ktls_tx_complete_pkts);
>  	} else {
> -		dev_kfree_skb_any(skb);
> -
> +		/* TAG needs to be calculated so, need to send complete record,
> +		 * free the original skb and send a new one.
> +		 */
>  		nskb = alloc_skb(0, GFP_KERNEL);
> -		if (!nskb)

Please add a patch that fixes the misuses of GFP_KERNEL as pointed out
by Julia. Make it a separate patch in this series, before this one.