On 06/12/2018 07:37 AM, Christoph Hellwig wrote:
>> Looks like the recent conversion from poll to poll_mask callback started
>> in 152524231023 ("net: add support for ->poll_mask in proto_ops") missed
>> to eventually convert kTLS, too: TCP's ->poll was converted over to the
>> ->poll_mask in commit 2c7d3dacebd4 ("net/tcp: convert to ->poll_mask")
>> and therefore kTLS wrongly saved the ->poll old one which is now NULL.
>
> Looks like this TLS code was added in the same cycle.
>
>> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
>> index 301f224..a127d61 100644
>> --- a/net/tls/tls_main.c
>> +++ b/net/tls/tls_main.c
>> @@ -712,7 +712,7 @@ static int __init tls_register(void)
>> build_protos(tls_prots[TLSV4], &tcp_prot);
>>
>> tls_sw_proto_ops = inet_stream_ops;
>> - tls_sw_proto_ops.poll = tls_sw_poll;
>> + tls_sw_proto_ops.poll_mask = tls_sw_poll_mask;
>> tls_sw_proto_ops.splice_read = tls_sw_splice_read;
>
> Not new in this patch, but copying ops vectors is a very bad idea, not
> only because your new instance can't be marked const and you thus open
> up exploit vectors. I would suggest to clean this up eventually.
Generally, agree with you. It could at minimum also be a __ro_after_init
candidate, at least the TLSV4 ops which wouldn't change. In v6 case though
it could be loaded as a module after TLS was initialized.
>> +__poll_t tls_sw_poll_mask(struct socket *sock, __poll_t events)
>> {
>> struct sock *sk = sock->sk;
>> struct tls_context *tls_ctx = tls_get_ctx(sk);
>> struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
>> + __poll_t mask;
>>
>> + /* Grab EPOLLOUT and EPOLLHUP from the underlying socket */
>> + mask = ctx->sk_poll_mask(sock, events);
>>
>> + /* Clear EPOLLIN bits, and set based on recv_pkt */
>> + mask &= ~(EPOLLIN | EPOLLRDNORM);
>> if (ctx->recv_pkt)
>> + mask |= EPOLLIN | EPOLLRDNORM;
>>
>> + return mask;
>
> So you call the underlying protocol method on the struct sock of
> the TLS code? Again not reall new in this patch, but how is this
> even supposed to work?
Yeah, patch doesn't change it, but reason is that TLS relies on kernel's
stream parser to determine TLS message boundary on ingress, so once a full
message got received only then we want to signal this to the user space
application. Latter skb is then held in ctx->recv_pkt via stream parser.
Thanks,
Daniel
