| Message ID | 20190226121235.20784-5-borisp@mellanox.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | tls: Fix issues in tls_device | expand |
> -----Original Message----- > From: Boris Pismenny <borisp@mellanox.com> > Sent: Tuesday, February 26, 2019 5:43 PM > To: aviadye@mellanox.com; davejwatson@fb.com; > john.fastabend@gmail.com; daniel@iogearbox.net; Vakul Garg > <vakul.garg@nxp.com>; netdev@vger.kernel.org > Cc: eranbe@mellanox.com; borisp@mellanox.com > Subject: [PATCH net 4/4] tls: Fix tls_device receive > > Currently, the receive function fails to handle records already decrypted by > the device due to the commit mentioned below. > > This commit advances the TLS record sequence number and prepares the > context to handle the next record. > > Fixes: fedf201e1296 ("net: tls: Refactor control message handling on recv") > Signed-off-by: Boris Pismenny <borisp@mellanox.com> > Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com> > --- > net/tls/tls_sw.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index > f515cd7e984e..85da10182d8d 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock *sk, > struct sk_buff *skb, > > return err; > } > - > - rxm->full_len -= padding_length(ctx, tls_ctx, skb); > - > - rxm->offset += prot->prepend_size; > - rxm->full_len -= prot->overhead_size; > - tls_advance_record_sn(sk, &tls_ctx->rx, version); > - ctx->decrypted = true; > - ctx->saved_data_ready(sk); > } else { > *zc = false; > } > > + rxm->full_len -= padding_length(ctx, tls_ctx, skb); > + rxm->offset += prot->prepend_size; > + rxm->full_len -= prot->overhead_size; > + tls_advance_record_sn(sk, &tls_ctx->rx, version); > + ctx->decrypted = true; > + ctx->saved_data_ready(sk); > + > return err; > } > > -- > 2.12.2 Reviewed-by: Vakul Garg <vakul.garg@nxp.com>
On 02/26/19 02:12 PM, Boris Pismenny wrote: > Currently, the receive function fails to handle records already > decrypted by the device due to the commit mentioned below. > > This commit advances the TLS record sequence number and prepares the context > to handle the next record. > > Fixes: fedf201e1296 ("net: tls: Refactor control message handling on recv") > Signed-off-by: Boris Pismenny <borisp@mellanox.com> > Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com> > --- > net/tls/tls_sw.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index f515cd7e984e..85da10182d8d 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb, > > return err; > } > - > - rxm->full_len -= padding_length(ctx, tls_ctx, skb); > - > - rxm->offset += prot->prepend_size; > - rxm->full_len -= prot->overhead_size; > - tls_advance_record_sn(sk, &tls_ctx->rx, version); > - ctx->decrypted = true; > - ctx->saved_data_ready(sk); > } else { > *zc = false; > } > > + rxm->full_len -= padding_length(ctx, tls_ctx, skb); > + rxm->offset += prot->prepend_size; > + rxm->full_len -= prot->overhead_size; > + tls_advance_record_sn(sk, &tls_ctx->rx, version); > + ctx->decrypted = true; > + ctx->saved_data_ready(sk); > + > return err; > } This breaks the tls.control_msg test: [ RUN ] tls.control_msg tls.c:764:tls.control_msg:Expected memcmp(buf, test_str, send_len) (18446744073709551614) == 0 (0) tls.c:777:tls.control_msg:Expected memcmp(buf, test_str, send_len) (18446744073709551614) == 0 (0) tls.control_msg: Test failed at step #8 So either control message handling needs to only call decrypt_skb_update once, or we need a new flag or something to handle the device case
> -----Original Message----- > From: Dave Watson <davejwatson@fb.com> > Sent: Wednesday, February 27, 2019 2:05 AM > To: Boris Pismenny <borisp@mellanox.com> > Cc: aviadye@mellanox.com; john.fastabend@gmail.com; > daniel@iogearbox.net; Vakul Garg <vakul.garg@nxp.com>; > netdev@vger.kernel.org; eranbe@mellanox.com > Subject: Re: [PATCH net 4/4] tls: Fix tls_device receive > > On 02/26/19 02:12 PM, Boris Pismenny wrote: > > Currently, the receive function fails to handle records already > > decrypted by the device due to the commit mentioned below. > > > > This commit advances the TLS record sequence number and prepares the > > context to handle the next record. > > > > Fixes: fedf201e1296 ("net: tls: Refactor control message handling on > > recv") > > Signed-off-by: Boris Pismenny <borisp@mellanox.com> > > Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com> > > --- > > net/tls/tls_sw.c | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index > > f515cd7e984e..85da10182d8d 100644 > > --- a/net/tls/tls_sw.c > > +++ b/net/tls/tls_sw.c > > @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock *sk, > > struct sk_buff *skb, > > > > return err; > > } > > - > > - rxm->full_len -= padding_length(ctx, tls_ctx, skb); > > - > > - rxm->offset += prot->prepend_size; > > - rxm->full_len -= prot->overhead_size; > > - tls_advance_record_sn(sk, &tls_ctx->rx, version); > > - ctx->decrypted = true; > > - ctx->saved_data_ready(sk); > > } else { > > *zc = false; > > } > > > > + rxm->full_len -= padding_length(ctx, tls_ctx, skb); > > + rxm->offset += prot->prepend_size; > > + rxm->full_len -= prot->overhead_size; > > + tls_advance_record_sn(sk, &tls_ctx->rx, version); > > + ctx->decrypted = true; > > + ctx->saved_data_ready(sk); > > + > > return err; > > } > > This breaks the tls.control_msg test: > > [ RUN ] tls.control_msg > tls.c:764:tls.control_msg:Expected memcmp(buf, test_str, send_len) > (18446744073709551614) == 0 (0) > tls.c:777:tls.control_msg:Expected memcmp(buf, test_str, send_len) > (18446744073709551614) == 0 (0) > tls.control_msg: Test failed at step #8 > > So either control message handling needs to only call decrypt_skb_update > once, or we need a new flag or something to handle the device case I prefer to remove variable 'decrypted' in context. This is no longer required as we already have an rx_list in context for storing decrypted records. So for any record which we decrypted but did not return to user space (e.g. for the case when user used recv() and it lead to decryption of non-data record), we should it in rx_list.
On 2/27/2019 5:08 AM, Vakul Garg wrote: > > >> -----Original Message----- >> From: Dave Watson <davejwatson@fb.com> >> Sent: Wednesday, February 27, 2019 2:05 AM >> To: Boris Pismenny <borisp@mellanox.com> >> Cc: aviadye@mellanox.com; john.fastabend@gmail.com; >> daniel@iogearbox.net; Vakul Garg <vakul.garg@nxp.com>; >> netdev@vger.kernel.org; eranbe@mellanox.com >> Subject: Re: [PATCH net 4/4] tls: Fix tls_device receive >> >> On 02/26/19 02:12 PM, Boris Pismenny wrote: >>> Currently, the receive function fails to handle records already >>> decrypted by the device due to the commit mentioned below. >>> >>> This commit advances the TLS record sequence number and prepares the >>> context to handle the next record. >>> >>> Fixes: fedf201e1296 ("net: tls: Refactor control message handling on >>> recv") >>> Signed-off-by: Boris Pismenny <borisp@mellanox.com> >>> Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com> >>> --- >>> net/tls/tls_sw.c | 15 +++++++-------- >>> 1 file changed, 7 insertions(+), 8 deletions(-) >>> >>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index >>> f515cd7e984e..85da10182d8d 100644 >>> --- a/net/tls/tls_sw.c >>> +++ b/net/tls/tls_sw.c >>> @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock *sk, >>> struct sk_buff *skb, >>> >>> return err; >>> } >>> - >>> - rxm->full_len -= padding_length(ctx, tls_ctx, skb); >>> - >>> - rxm->offset += prot->prepend_size; >>> - rxm->full_len -= prot->overhead_size; >>> - tls_advance_record_sn(sk, &tls_ctx->rx, version); >>> - ctx->decrypted = true; >>> - ctx->saved_data_ready(sk); >>> } else { >>> *zc = false; >>> } >>> >>> + rxm->full_len -= padding_length(ctx, tls_ctx, skb); >>> + rxm->offset += prot->prepend_size; >>> + rxm->full_len -= prot->overhead_size; >>> + tls_advance_record_sn(sk, &tls_ctx->rx, version); >>> + ctx->decrypted = true; >>> + ctx->saved_data_ready(sk); >>> + >>> return err; >>> } >> >> This breaks the tls.control_msg test: >> >> [ RUN ] tls.control_msg >> tls.c:764:tls.control_msg:Expected memcmp(buf, test_str, send_len) >> (18446744073709551614) == 0 (0) >> tls.c:777:tls.control_msg:Expected memcmp(buf, test_str, send_len) >> (18446744073709551614) == 0 (0) >> tls.control_msg: Test failed at step #8 >> >> So either control message handling needs to only call decrypt_skb_update >> once, or we need a new flag or something to handle the device case > > I prefer to remove variable 'decrypted' in context. > This is no longer required as we already have an rx_list in context for storing decrypted records. > So for any record which we decrypted but did not return to user space > (e.g. for the case when user used recv() and it lead to decryption of non-data record), we should > it in rx_list. > IMO this is inappropriate here, because packets decrypted by tls_device are ready to be received, and there is no reason to bounce them through the rx_list.
On 2/26/2019 10:34 PM, Dave Watson wrote: > On 02/26/19 02:12 PM, Boris Pismenny wrote: >> Currently, the receive function fails to handle records already >> decrypted by the device due to the commit mentioned below. >> >> This commit advances the TLS record sequence number and prepares the context >> to handle the next record. >> >> Fixes: fedf201e1296 ("net: tls: Refactor control message handling on recv") >> Signed-off-by: Boris Pismenny <borisp@mellanox.com> >> Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com> >> --- >> net/tls/tls_sw.c | 15 +++++++-------- >> 1 file changed, 7 insertions(+), 8 deletions(-) >> >> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c >> index f515cd7e984e..85da10182d8d 100644 >> --- a/net/tls/tls_sw.c >> +++ b/net/tls/tls_sw.c >> @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb, >> >> return err; >> } >> - >> - rxm->full_len -= padding_length(ctx, tls_ctx, skb); >> - >> - rxm->offset += prot->prepend_size; >> - rxm->full_len -= prot->overhead_size; >> - tls_advance_record_sn(sk, &tls_ctx->rx, version); >> - ctx->decrypted = true; >> - ctx->saved_data_ready(sk); >> } else { >> *zc = false; >> } >> >> + rxm->full_len -= padding_length(ctx, tls_ctx, skb); >> + rxm->offset += prot->prepend_size; >> + rxm->full_len -= prot->overhead_size; >> + tls_advance_record_sn(sk, &tls_ctx->rx, version); >> + ctx->decrypted = true; >> + ctx->saved_data_ready(sk); >> + >> return err; >> } > > This breaks the tls.control_msg test: > > [ RUN ] tls.control_msg > tls.c:764:tls.control_msg:Expected memcmp(buf, test_str, send_len) (18446744073709551614) == 0 (0) > tls.c:777:tls.control_msg:Expected memcmp(buf, test_str, send_len) (18446744073709551614) == 0 (0) > tls.control_msg: Test failed at step #8 > > So either control message handling needs to only call > decrypt_skb_update once, or we need a new flag or something to handle > the device case > Thanks for raising this, I'm not used to the kselftests yet. I've refactored the code here to get this working. Will send V2 soon.
> -----Original Message----- > From: Boris Pismenny <borisp@mellanox.com> > Sent: Wednesday, February 27, 2019 8:54 PM > To: Vakul Garg <vakul.garg@nxp.com>; Dave Watson > <davejwatson@fb.com> > Cc: Aviad Yehezkel <aviadye@mellanox.com>; john.fastabend@gmail.com; > daniel@iogearbox.net; netdev@vger.kernel.org; Eran Ben Elisha > <eranbe@mellanox.com> > Subject: Re: [PATCH net 4/4] tls: Fix tls_device receive > > > > On 2/27/2019 5:08 AM, Vakul Garg wrote: > > > > > >> -----Original Message----- > >> From: Dave Watson <davejwatson@fb.com> > >> Sent: Wednesday, February 27, 2019 2:05 AM > >> To: Boris Pismenny <borisp@mellanox.com> > >> Cc: aviadye@mellanox.com; john.fastabend@gmail.com; > >> daniel@iogearbox.net; Vakul Garg <vakul.garg@nxp.com>; > >> netdev@vger.kernel.org; eranbe@mellanox.com > >> Subject: Re: [PATCH net 4/4] tls: Fix tls_device receive > >> > >> On 02/26/19 02:12 PM, Boris Pismenny wrote: > >>> Currently, the receive function fails to handle records already > >>> decrypted by the device due to the commit mentioned below. > >>> > >>> This commit advances the TLS record sequence number and prepares the > >>> context to handle the next record. > >>> > >>> Fixes: fedf201e1296 ("net: tls: Refactor control message handling on > >>> recv") > >>> Signed-off-by: Boris Pismenny <borisp@mellanox.com> > >>> Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com> > >>> --- > >>> net/tls/tls_sw.c | 15 +++++++-------- > >>> 1 file changed, 7 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index > >>> f515cd7e984e..85da10182d8d 100644 > >>> --- a/net/tls/tls_sw.c > >>> +++ b/net/tls/tls_sw.c > >>> @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock > >>> *sk, struct sk_buff *skb, > >>> > >>> return err; > >>> } > >>> - > >>> - rxm->full_len -= padding_length(ctx, tls_ctx, skb); > >>> - > >>> - rxm->offset += prot->prepend_size; > >>> - rxm->full_len -= prot->overhead_size; > >>> - tls_advance_record_sn(sk, &tls_ctx->rx, version); > >>> - ctx->decrypted = true; > >>> - ctx->saved_data_ready(sk); > >>> } else { > >>> *zc = false; > >>> } > >>> > >>> + rxm->full_len -= padding_length(ctx, tls_ctx, skb); > >>> + rxm->offset += prot->prepend_size; > >>> + rxm->full_len -= prot->overhead_size; > >>> + tls_advance_record_sn(sk, &tls_ctx->rx, version); > >>> + ctx->decrypted = true; > >>> + ctx->saved_data_ready(sk); > >>> + > >>> return err; > >>> } > >> > >> This breaks the tls.control_msg test: > >> > >> [ RUN ] tls.control_msg > >> tls.c:764:tls.control_msg:Expected memcmp(buf, test_str, send_len) > >> (18446744073709551614) == 0 (0) > >> tls.c:777:tls.control_msg:Expected memcmp(buf, test_str, send_len) > >> (18446744073709551614) == 0 (0) > >> tls.control_msg: Test failed at step #8 > >> > >> So either control message handling needs to only call > >> decrypt_skb_update once, or we need a new flag or something to handle > >> the device case > > > > I prefer to remove variable 'decrypted' in context. > > This is no longer required as we already have an rx_list in context for > storing decrypted records. > > So for any record which we decrypted but did not return to user space > > (e.g. for the case when user used recv() and it lead to decryption of > > non-data record), we should it in rx_list. > > > > IMO this is inappropriate here, because packets decrypted by tls_device are > ready to be received, and there is no reason to bounce them through the > rx_list. My point was about preventing tls_sw_recvmsg() from calling decrypt_skb_update() with an already decrypted record. The test case failed because an already decrypted record got dequeued and passed to decrypt_skb_update() from tls_sw_recvmsg(). For packets decrypted by device, a check using skb->decrypted should be enough. For now, I think your patch is ok. I can submit a simplification patch for removing 'decrypted' from tls context later.
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index f515cd7e984e..85da10182d8d 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb, return err; } - - rxm->full_len -= padding_length(ctx, tls_ctx, skb); - - rxm->offset += prot->prepend_size; - rxm->full_len -= prot->overhead_size; - tls_advance_record_sn(sk, &tls_ctx->rx, version); - ctx->decrypted = true; - ctx->saved_data_ready(sk); } else { *zc = false; } + rxm->full_len -= padding_length(ctx, tls_ctx, skb); + rxm->offset += prot->prepend_size; + rxm->full_len -= prot->overhead_size; + tls_advance_record_sn(sk, &tls_ctx->rx, version); + ctx->decrypted = true; + ctx->saved_data_ready(sk); + return err; }
Currently, the receive function fails to handle records already decrypted by the device due to the commit mentioned below. This commit advances the TLS record sequence number and prepares the context to handle the next record. Fixes: fedf201e1296 ("net: tls: Refactor control message handling on recv") Signed-off-by: Boris Pismenny <borisp@mellanox.com> Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com> --- net/tls/tls_sw.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)