netdev - Re: [PATCH V2 net-next 06/14] net/tls: Add generic NIC offload infrastructure

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <17935bc6-f82e-482c-e198-cba500c698a3@mellanox.com>
Date:   Thu, 22 Mar 2018 14:45:12 +0200
From:   Boris Pismenny <borisp@...lanox.com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>
Cc:     netdev@...r.kernel.org, Dave Watson <davejwatson@...com>,
        Ilya Lesokhin <ilyal@...lanox.com>,
        Aviad Yehezkel <aviadye@...lanox.com>
Subject: Re: [PATCH V2 net-next 06/14] net/tls: Add generic NIC offload
 infrastructure



On 3/21/2018 11:10 PM, Eric Dumazet wrote:
> 
> 
> On 03/21/2018 02:01 PM, Saeed Mahameed wrote:
>> From: Ilya Lesokhin <ilyal@...lanox.com>
>>
>> This patch adds a generic infrastructure to offload TLS crypto to a
> 
> ...
> 
>> +
>> +static inline int tls_push_record(struct sock *sk,
>> +				  struct tls_context *ctx,
>> +				  struct tls_offload_context *offload_ctx,
>> +				  struct tls_record_info *record,
>> +				  struct page_frag *pfrag,
>> +				  int flags,
>> +				  unsigned char record_type)
>> +{
>> +	skb_frag_t *frag;
>> +	struct tcp_sock *tp = tcp_sk(sk);
>> +	struct page_frag fallback_frag;
>> +	struct page_frag  *tag_pfrag = pfrag;
>> +	int i;
>> +
>> +	/* fill prepand */
>> +	frag = &record->frags[0];
>> +	tls_fill_prepend(ctx,
>> +			 skb_frag_address(frag),
>> +			 record->len - ctx->prepend_size,
>> +			 record_type);
>> +
>> +	if (unlikely(!skb_page_frag_refill(ctx->tag_size, pfrag, GFP_KERNEL))) {
>> +		/* HW doesn't care about the data in the tag
>> +		 * so in case pfrag has no room
>> +		 * for a tag and we can't allocate a new pfrag
>> +		 * just use the page in the first frag
>> +		 * rather then write a complicated fall back code.
>> +		 */
>> +		tag_pfrag = &fallback_frag;
>> +		tag_pfrag->page = skb_frag_page(frag);
>> +		tag_pfrag->offset = 0;
>> +	}
>> +
> 
> If HW does not care, why even trying to call skb_page_frag_refill() ?
> 

There's no particular reason for allocating memory here. I'll remove it 
for V3.

> If you remove it, then we remove one seldom used path and might uncover bugs
> 
> This part looks very suspect to me, to be honest.
> 

HW doesn't care, because it generates the tag, and nothing along the 
network stack touches the data here.

Would you prefer that we allocate it anyway and wait for memory if it is 
not available?