[net-next v6 0/2] Minor code cleanup patches

[net-next v6 0/2] Minor code cleanup patches

[Vakul Garg]

This patch series improves tls_sw.c code by:

1) Using correct socket callback for flagging data availability.
2) Removing redundant variable assignments and wakeup callbacks.


Vakul Garg (2):
  net/tls: Use socket data_ready callback on record availability
  net/tls: Remove redundant variable assignments and wakeup

 net/tls/tls_sw.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

-- 
2.13.6

[net-next v6 1/2] net/tls: Use socket data_ready callback on record availability

[Vakul Garg]

On receipt of a complete tls record, use socket's saved data_ready
callback instead of state_change callback.

Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
 net/tls/tls_sw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 0c2d029c9d4c..fee1240eff92 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1028,7 +1028,7 @@ static void tls_queue(struct strparser *strp, struct sk_buff *skb)
 	ctx->recv_pkt = skb;
 	strp_pause(strp);
 
-	strp->sk->sk_state_change(strp->sk);
+	ctx->saved_data_ready(strp->sk);
 }
 
 static void tls_data_ready(struct sock *sk)
-- 
2.13.6

Re: [net-next v6 1/2] net/tls: Use socket data_ready callback on record availability

[David Miller]

From: Vakul Garg <vakul.garg@nxp.com>
Date: Tue, 24 Jul 2018 15:44:02 +0530

> On receipt of a complete tls record, use socket's saved data_ready
> callback instead of state_change callback.
> 
> Signed-off-by: Vakul Garg <vakul.garg@nxp.com>

I don't think this is correct.

Here, the stream parser has given us a complete TLS record.

But we haven't decrypted this packet yet.  It sits on the
stream parser's queue to be processed by tls_sw_recvmsg(),
not the saved socket's receive queue.

RE: [net-next v6 1/2] net/tls: Use socket data_ready callback on record availability

[Vakul Garg]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, July 25, 2018 1:43 AM
> To: Vakul Garg <vakul.garg@nxp.com>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com
> Subject: Re: [net-next v6 1/2] net/tls: Use socket data_ready callback on
> record availability
> 
> From: Vakul Garg <vakul.garg@nxp.com>
> Date: Tue, 24 Jul 2018 15:44:02 +0530
> 
> > On receipt of a complete tls record, use socket's saved data_ready
> > callback instead of state_change callback.
> >
> > Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
> 
> I don't think this is correct.
> 
> Here, the stream parser has given us a complete TLS record.
> 
> But we haven't decrypted this packet yet.  It sits on the stream parser's
> queue to be processed by tls_sw_recvmsg(), not the saved socket's receive
> queue.

I understand that at this point in code, the TLS record is still queued in encrypted
state. But the decryption happens inline when tls_sw_recvmsg() gets invokved.
So it should be ok to notify the  waiting context about the availability of data as soon
as we could collect a full TLS record.

For new data availability notification, sk_data_ready callback should be more
more appropriate. It points to sock_def_readable() which wakes up specifically for
EPOLLIN event. 

This is in contrast to the socket callback sk_state_change which points
to sock_def_wakeup() which issues a wakeup unconditionally (without event mask).

RE: [net-next v6 1/2] net/tls: Use socket data_ready callback on record availability

[Vakul Garg]

Hi David

Could you please correct me if my counter-reasoning behind changing the socket callback is wrong?

Thanks & Regards

Vakul

> -----Original Message-----
> From: Vakul Garg
> Sent: Wednesday, July 25, 2018 11:22 AM
> To: David Miller <davem@davemloft.net>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com
> Subject: RE: [net-next v6 1/2] net/tls: Use socket data_ready callback on
> record availability
> 
> 
> 
> > -----Original Message-----
> > From: David Miller [mailto:davem@davemloft.net]
> > Sent: Wednesday, July 25, 2018 1:43 AM
> > To: Vakul Garg <vakul.garg@nxp.com>
> > Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com;
> > davejwatson@fb.com
> > Subject: Re: [net-next v6 1/2] net/tls: Use socket data_ready callback
> > on record availability
> >
> > From: Vakul Garg <vakul.garg@nxp.com>
> > Date: Tue, 24 Jul 2018 15:44:02 +0530
> >
> > > On receipt of a complete tls record, use socket's saved data_ready
> > > callback instead of state_change callback.
> > >
> > > Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
> >
> > I don't think this is correct.
> >
> > Here, the stream parser has given us a complete TLS record.
> >
> > But we haven't decrypted this packet yet.  It sits on the stream
> > parser's queue to be processed by tls_sw_recvmsg(), not the saved
> > socket's receive queue.
> 
> I understand that at this point in code, the TLS record is still queued in
> encrypted state. But the decryption happens inline when tls_sw_recvmsg()
> gets invokved.
> So it should be ok to notify the  waiting context about the availability of data
> as soon as we could collect a full TLS record.
> 
> For new data availability notification, sk_data_ready callback should be more
> more appropriate. It points to sock_def_readable() which wakes up
> specifically for EPOLLIN event.
> 
> This is in contrast to the socket callback sk_state_change which points to
> sock_def_wakeup() which issues a wakeup unconditionally (without event
> mask).

Re: [net-next v6 1/2] net/tls: Use socket data_ready callback on record availability

[David Miller]

From: Vakul Garg <vakul.garg@nxp.com>
Date: Sun, 29 Jul 2018 06:01:29 +0000

> Could you please correct me if my counter-reasoning behind changing
> the socket callback is wrong?

Ok, after stufying the code a bit I agree with your analysis.

RE: [net-next v6 1/2] net/tls: Use socket data_ready callback on record availability

[Vakul Garg]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Sunday, July 29, 2018 11:48 AM
> To: Vakul Garg <vakul.garg@nxp.com>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com
> Subject: Re: [net-next v6 1/2] net/tls: Use socket data_ready callback on
> record availability
> 
> From: Vakul Garg <vakul.garg@nxp.com>
> Date: Sun, 29 Jul 2018 06:01:29 +0000
> 
> > Could you please correct me if my counter-reasoning behind changing
> > the socket callback is wrong?
> 
> Ok, after stufying the code a bit I agree with your analysis.

Thanks.
Kindly advise, if I need to resubmit/rebase the patch.
 

[net-next v6 2/2] net/tls: Remove redundant variable assignments and wakeup

[Vakul Garg]

In function decrypt_skb_update(), the assignment to tls receive context
variable 'decrypted' is redundant as the same is being done in function
tls_sw_recvmsg() after calling decrypt_skb_update(). Also calling callback
function to wakeup processes sleeping on socket data availability is
useless as decrypt_skb_update() is invoked from user processes only. This
patch cleans these up.

Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
 net/tls/tls_sw.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index fee1240eff92..6c71da7b147f 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -679,8 +679,6 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 	rxm->offset += tls_ctx->rx.prepend_size;
 	rxm->full_len -= tls_ctx->rx.overhead_size;
 	tls_advance_record_sn(sk, &tls_ctx->rx);
-	ctx->decrypted = true;
-	ctx->saved_data_ready(sk);
 
 	return err;
 }
-- 
2.13.6