netdev - Re: [PATCH net 2/4] tls: Fix write space handling

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: <c38ba564-bb1e-13cb-e91f-ddd29527dc61@mellanox.com>
Date:   Mon, 11 Mar 2019 15:59:24 +0000
From:   Boris Pismenny <borisp@...lanox.com>
To:     Vakul Garg <vakul.garg@....com>,
        Aviad Yehezkel <aviadye@...lanox.com>,
        "davejwatson@...com" <davejwatson@...com>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Eran Ben Elisha <eranbe@...lanox.com>
Subject: Re: [PATCH net 2/4] tls: Fix write space handling

>>>> a/net/tls/tls_main.c b/net/tls/tls_main.c index
>>>> 7e05af75536d..11c1980a75cb 100644
>>>> --- a/net/tls/tls_main.c
>>>> +++ b/net/tls/tls_main.c
>>>> @@ -212,7 +212,7 @@ int tls_push_partial_record(struct sock *sk,
>>>> struct tls_context *ctx,
>>>>    static void tls_write_space(struct sock *sk)
>>>>    {
>>>>    	struct tls_context *ctx = tls_get_ctx(sk);
>>>> -	struct tls_sw_context_tx *tx_ctx = tls_sw_ctx_tx(ctx);
>>>> +	int rc;
>>>>
>>>>    	/* If in_tcp_sendpages call lower protocol write space handler
>>>>    	 * to ensure we wake up any waiting operations there. For example
>>>> @@ -223,14 +223,15 @@ static void tls_write_space(struct sock *sk)
>>>>    		return;
>>>>    	}
>>>>
>>>> -	/* Schedule the transmission if tx list is ready */
>>>> -	if (is_tx_ready(tx_ctx) && !sk->sk_write_pending) {
>>>> -		/* Schedule the transmission */
>>>> -		if (!test_and_set_bit(BIT_TX_SCHEDULED, &tx_ctx-
>>>>> tx_bitmask))
>>>> -			schedule_delayed_work(&tx_ctx->tx_work.work, 0);
>>>> -	}
>>>> +#ifdef CONFIG_TLS_DEVICE
>>>> +	if (ctx->tx_conf == TLS_HW)
>>>> +		rc = tls_device_write_space(sk, ctx);
>>>> +	else
>>>> +#endif
>>>> +		rc = tls_sw_write_space(sk, ctx);
>>>>
>>>> -	ctx->sk_write_space(sk);
>>>> +	if (!rc)
>>>
>>> Why do we need to check 'rc'?
>>>
>>> If it is required, then ' ctx->sk_write_space(sk)' can move to
>>> tls_device_write_space() since  tls_sw_write_space() always returns '0'.
>>>
>>
>> It is not necessary in the software code path due to the delayed work that is
>> there. But, we need in the device flow. I'll move it there.
>>
>   
> Removal of ctx->sk_write_space(sk) has broken software code flow.
> The ktls send stops and user space application waits infinitely.
> When tls_write_space() gets invoked tcp has been able to transmit some data.
> Shouldn't we unconditionally call ctx->sk_write_space() in order to inform
> user space application about availability of buffer space?
> 
> Please advise. I would submit the patch.

AFAIU, the code in the software path calls ctx->sk_write_space in its 
schedule work which eventually calls tls_push_sg. Since this flow is 
asynchronous, I thought it was best to postpone the notification and let 
the work handle it.

> 
>>
>>>> +		ctx->sk_write_space(sk);
>>>>    }
>>>>