Date:   Tue, 26 Feb 2019 14:57:38 +0000
From:   Vakul Garg <vakul.garg@....com>
To:     Boris Pismenny <borisp@...lanox.com>,
        "aviadye@...lanox.com" <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:     "eranbe@...lanox.com" <eranbe@...lanox.com>
Subject: RE: [PATCH net 1/4] tls: Fix tls_device handling of partial records



> -----Original Message-----
> From: Boris Pismenny <borisp@...lanox.com>
> Sent: Tuesday, February 26, 2019 5:43 PM
> To: aviadye@...lanox.com; davejwatson@...com;
> john.fastabend@...il.com; daniel@...earbox.net; Vakul Garg
> <vakul.garg@....com>; netdev@...r.kernel.org
> Cc: eranbe@...lanox.com; borisp@...lanox.com
> Subject: [PATCH net 1/4] tls: Fix tls_device handling of partial records
> 
> Cleanup the handling of partial records while fixing a bug where the
> tls_push_pending_closed_record function is using the software tls
> context instead of the hardware context.

Can you provide details of what cleanup has been done?
I see that we got rid of concept of 'TLS_PENDING_CLOSED_RECORD'.
I vaguely remember that at one point in time, it seemed to me redundant.
But I was not sure. Please confirm if it is the case.

Can this patch be split into two? One for the cleanup and one for the bug.

> 
> The bug resulted in the following crash:
> [   88.791229] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000000
> [   88.793271] #PF error: [normal kernel read fault]
> [   88.794449] PGD 800000022a426067 P4D 800000022a426067 PUD
> 22a156067 PMD 0
> [   88.795958] Oops: 0000 [#1] SMP PTI
> [   88.796884] CPU: 2 PID: 4973 Comm: openssl Not tainted 5.0.0-rc4+ #3
> [   88.798314] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS Bochs 01/01/2011
> [   88.800067] RIP: 0010:tls_tx_records+0xef/0x1d0 [tls]
> [   88.801256] Code: 00 02 48 89 43 08 e8 a0 0b 96 d9 48 89 df e8 48 dd
> 4d d9 4c 89 f8 4d 8b bf 98 00 00 00 48 05 98 00 00 00 48 89 04 24 49 39
> c7 <49> 8b 1f 4d 89 fd 0f 84 af 00 00 00 41 8b 47 10 85 c0 0f 85 8d 00
> [   88.805179] RSP: 0018:ffffbd888186fca8 EFLAGS: 00010213
> [   88.806458] RAX: ffff9af1ed657c98 RBX: ffff9af1e88a1980 RCX:
> 0000000000000000
> [   88.808050] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> ffff9af1e88a1980
> [   88.809724] RBP: ffff9af1e88a1980 R08: 0000000000000017 R09:
> ffff9af1ebeeb700
> [   88.811294] R10: 0000000000000000 R11: 0000000000000000 R12:
> 0000000000000000
> [   88.812917] R13: ffff9af1e88a1980 R14: ffff9af1ec13f800 R15:
> 0000000000000000
> [   88.814506] FS:  00007fcad2240740(0000) GS:ffff9af1f7880000(0000)
> knlGS:0000000000000000
> [   88.816337] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   88.817717] CR2: 0000000000000000 CR3: 0000000228b3e000 CR4:
> 00000000001406e0
> [   88.819328] Call Trace:
> [   88.820123]  tls_push_data+0x628/0x6a0 [tls]
> [   88.821283]  ? remove_wait_queue+0x20/0x60
> [   88.822383]  ? n_tty_read+0x683/0x910
> [   88.823363]  tls_device_sendmsg+0x53/0xa0 [tls]
> [   88.824505]  sock_sendmsg+0x36/0x50
> [   88.825492]  sock_write_iter+0x87/0x100
> [   88.826521]  __vfs_write+0x127/0x1b0
> [   88.827499]  vfs_write+0xad/0x1b0
> [   88.828454]  ksys_write+0x52/0xc0
> [   88.829378]  do_syscall_64+0x5b/0x180
> [   88.830369]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [   88.831603] RIP: 0033:0x7fcad1451680
> 
> [ 1248.470626] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000000
> [ 1248.472564] #PF error: [normal kernel read fault]
> [ 1248.473790] PGD 0 P4D 0
> [ 1248.474642] Oops: 0000 [#1] SMP PTI
> [ 1248.475651] CPU: 3 PID: 7197 Comm: openssl Tainted: G           OE 5.0.0-
> rc4+ #3
> [ 1248.477426] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS Bochs 01/01/2011
> [ 1248.479310] RIP: 0010:tls_tx_records+0x110/0x1f0 [tls]
> [ 1248.480644] Code: 00 02 48 89 43 08 e8 4f cb 63 d7 48 89 df e8 f7 9c
> 1b d7 4c 89 f8 4d 8b bf 98 00 00 00 48 05 98 00 00 00 48 89 04 24 49 39
> c7 <49> 8b 1f 4d 89 fd 0f 84 af 00 00 00 41 8b 47 10 85 c0 0f 85 8d 00
> [ 1248.484825] RSP: 0018:ffffaa0a41543c08 EFLAGS: 00010213
> [ 1248.486154] RAX: ffff955a2755dc98 RBX: ffff955a36031980 RCX:
> 0000000000000006
> [ 1248.487855] RDX: 0000000000000000 RSI: 000000000000002b RDI:
> 0000000000000286
> [ 1248.489524] RBP: ffff955a36031980 R08: 0000000000000000 R09:
> 00000000000002b1
> [ 1248.491394] R10: 0000000000000003 R11: 00000000ad55ad55 R12:
> 0000000000000000
> [ 1248.493162] R13: 0000000000000000 R14: ffff955a2abe6c00 R15:
> 0000000000000000
> [ 1248.494923] FS:  0000000000000000(0000) GS:ffff955a378c0000(0000)
> knlGS:0000000000000000
> [ 1248.496847] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1248.498357] CR2: 0000000000000000 CR3: 000000020c40e000 CR4:
> 00000000001406e0
> [ 1248.500136] Call Trace:
> [ 1248.500998]  ? tcp_check_oom+0xd0/0xd0
> [ 1248.502106]  tls_sk_proto_close+0x127/0x1e0 [tls]
> [ 1248.503411]  inet_release+0x3c/0x60
> [ 1248.504530]  __sock_release+0x3d/0xb0
> [ 1248.505611]  sock_close+0x11/0x20
> [ 1248.506612]  __fput+0xb4/0x220
> [ 1248.507559]  task_work_run+0x88/0xa0
> [ 1248.508617]  do_exit+0x2cb/0xbc0
> [ 1248.509597]  ? core_sys_select+0x17a/0x280
> [ 1248.510740]  do_group_exit+0x39/0xb0
> [ 1248.511789]  get_signal+0x1d0/0x630
> [ 1248.512823]  do_signal+0x36/0x620
> [ 1248.513822]  exit_to_usermode_loop+0x5c/0xc6
> [ 1248.515003]  do_syscall_64+0x157/0x180
> [ 1248.516094]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 1248.517456] RIP: 0033:0x7fb398bd3f53
> [ 1248.518537] Code: Bad RIP value.
> 
> Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption of records
> for performance")
> Signed-off-by: Boris Pismenny <borisp@...lanox.com>
> Signed-off-by: Eran Ben Elisha <eranbe@...lanox.com>
> ---
>  include/net/tls.h    | 20 ++++----------------
>  net/tls/tls_device.c |  9 +++++----
>  net/tls/tls_main.c   | 13 -------------
>  3 files changed, 9 insertions(+), 33 deletions(-)
> 
> diff --git a/include/net/tls.h b/include/net/tls.h
> index 9f4117ae2297..a528a082da73 100644
> --- a/include/net/tls.h
> +++ b/include/net/tls.h
> @@ -199,10 +199,6 @@ struct tls_offload_context_tx {
>  	(ALIGN(sizeof(struct tls_offload_context_tx), sizeof(void *)) +        \
>  	 TLS_DRIVER_STATE_SIZE)
> 
> -enum {
> -	TLS_PENDING_CLOSED_RECORD
> -};
> -
>  struct cipher_context {
>  	char *iv;
>  	char *rec_seq;
> @@ -335,17 +331,14 @@ int tls_push_sg(struct sock *sk, struct tls_context
> *ctx,
>  int tls_push_partial_record(struct sock *sk, struct tls_context *ctx,
>  			    int flags);
> 
> -int tls_push_pending_closed_record(struct sock *sk, struct tls_context *ctx,
> -				   int flags, long *timeo);
> -
>  static inline struct tls_msg *tls_msg(struct sk_buff *skb)
>  {
>  	return (struct tls_msg *)strp_msg(skb);
>  }
> 
> -static inline bool tls_is_pending_closed_record(struct tls_context *ctx)
> +static inline bool tls_is_partially_sent_record(struct tls_context *ctx)
>  {
> -	return test_bit(TLS_PENDING_CLOSED_RECORD, &ctx->flags);
> +	return !!ctx->partially_sent_record;
>  }
> 
>  static inline int tls_complete_pending_work(struct sock *sk,
> @@ -357,17 +350,12 @@ static inline int tls_complete_pending_work(struct
> sock *sk,
>  	if (unlikely(sk->sk_write_pending))
>  		rc = wait_on_pending_writer(sk, timeo);
> 
> -	if (!rc && tls_is_pending_closed_record(ctx))
> -		rc = tls_push_pending_closed_record(sk, ctx, flags, timeo);
> +	if (!rc && tls_is_partially_sent_record(ctx))
> +		rc = tls_push_partial_record(sk, ctx, flags);
> 
>  	return rc;
>  }
> 
> -static inline bool tls_is_partially_sent_record(struct tls_context *ctx)
> -{
> -	return !!ctx->partially_sent_record;
> -}
> -
>  static inline bool tls_is_pending_open_record(struct tls_context *tls_ctx)
>  {
>  	return tls_ctx->pending_open_record_frags;
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index a5c17c47d08a..3e5e8e021a87 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -271,7 +271,6 @@ static int tls_push_record(struct sock *sk,
>  	list_add_tail(&record->list, &offload_ctx->records_list);
>  	spin_unlock_irq(&offload_ctx->lock);
>  	offload_ctx->open_record = NULL;
> -	set_bit(TLS_PENDING_CLOSED_RECORD, &ctx->flags);
>  	tls_advance_record_sn(sk, &ctx->tx, ctx->crypto_send.info.version);
> 
>  	for (i = 0; i < record->num_frags; i++) {
> @@ -368,9 +367,11 @@ static int tls_push_data(struct sock *sk,
>  		return -sk->sk_err;
> 
>  	timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT);
> -	rc = tls_complete_pending_work(sk, tls_ctx, flags, &timeo);
> -	if (rc < 0)
> -		return rc;
> +	if (tls_is_partially_sent_record(tls_ctx)) {
> +		rc = tls_push_partial_record(sk, tls_ctx, flags);
> +		if (rc < 0)
> +			return rc;
> +	}
> 
>  	pfrag = sk_page_frag(sk);
> 
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index caff15b2f9b2..7e05af75536d 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -209,19 +209,6 @@ int tls_push_partial_record(struct sock *sk, struct
> tls_context *ctx,
>  	return tls_push_sg(sk, ctx, sg, offset, flags);
>  }
> 
> -int tls_push_pending_closed_record(struct sock *sk,
> -				   struct tls_context *tls_ctx,
> -				   int flags, long *timeo)
> -{
> -	struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> -
> -	if (tls_is_partially_sent_record(tls_ctx) ||
> -	    !list_empty(&ctx->tx_list))
> -		return tls_tx_records(sk, flags);
> -	else
> -		return tls_ctx->push_pending_record(sk, flags);
> -}
> -
>  static void tls_write_space(struct sock *sk)
>  {
>  	struct tls_context *ctx = tls_get_ctx(sk);
> --
> 2.12.2

Powered by blists - more mailing lists