[PATCH net-next] tls: Fix socket mem accounting error under async encryption

[PATCH net-next] tls: Fix socket mem accounting error under async encryption

[Vakul Garg]

Current async encryption implementation sometimes showed up socket
memory accounting error during socket close. This results in kernel
warning calltrace. The root cause of the problem is that socket var
sk_forward_alloc gets corrupted due to access in sk_mem_charge()
and sk_mem_uncharge() being invoked from multiple concurrent contexts
in multicore processor. The apis sk_mem_charge() and sk_mem_uncharge()
are called from functions alloc_plaintext_sg(), free_sg() etc. It is
required that memory accounting apis are called under a socket lock.

The plaintext sg data sent for encryption is freed using free_sg() in
tls_encryption_done(). It is wrong to call free_sg() from this function.
This is because this function may run in irq context. We cannot acquire
socket lock in this function.

We remove calling of function free_sg() for plaintext data from
tls_encryption_done() and defer freeing up of plaintext data to the time
when the record is picked up from tx_list and transmitted/freed. When
tls_tx_records() gets called, socket is already locked and thus there is
no concurrent access problem.

Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption")
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
 net/tls/tls_sw.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index bf03f32aa983..406d3bb98818 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -353,6 +353,9 @@ int tls_tx_records(struct sock *sk, int flags)
 		 * Remove the head of tx_list
 		 */
 		list_del(&rec->list);
+		free_sg(sk, rec->sg_plaintext_data,
+			&rec->sg_plaintext_num_elem, &rec->sg_plaintext_size);
+
 		kfree(rec);
 	}
 
@@ -371,6 +374,10 @@ int tls_tx_records(struct sock *sk, int flags)
 				goto tx_err;
 
 			list_del(&rec->list);
+			free_sg(sk, rec->sg_plaintext_data,
+				&rec->sg_plaintext_num_elem,
+				&rec->sg_plaintext_size);
+
 			kfree(rec);
 		} else {
 			break;
@@ -399,8 +406,6 @@ static void tls_encrypt_done(struct crypto_async_request *req, int err)
 	rec->sg_encrypted_data[0].offset -= tls_ctx->tx.prepend_size;
 	rec->sg_encrypted_data[0].length += tls_ctx->tx.prepend_size;
 
-	free_sg(sk, rec->sg_plaintext_data,
-		&rec->sg_plaintext_num_elem, &rec->sg_plaintext_size);
 
 	/* Free the record if error is previously set on socket */
 	if (err || sk->sk_err) {
@@ -523,9 +528,6 @@ static int tls_push_record(struct sock *sk, int flags,
 	if (rc == -EINPROGRESS)
 		return -EINPROGRESS;
 
-	free_sg(sk, rec->sg_plaintext_data, &rec->sg_plaintext_num_elem,
-		&rec->sg_plaintext_size);
-
 	if (rc < 0) {
 		tls_err_abort(sk, EBADMSG);
 		return rc;
@@ -1566,6 +1568,11 @@ void tls_sw_free_resources_tx(struct sock *sk)
 
 		rec = list_first_entry(&ctx->tx_list,
 				       struct tls_rec, list);
+
+		free_sg(sk, rec->sg_plaintext_data,
+			&rec->sg_plaintext_num_elem,
+			&rec->sg_plaintext_size);
+
 		list_del(&rec->list);
 		kfree(rec);
 	}
@@ -1575,6 +1582,10 @@ void tls_sw_free_resources_tx(struct sock *sk)
 			&rec->sg_encrypted_num_elem,
 			&rec->sg_encrypted_size);
 
+		free_sg(sk, rec->sg_plaintext_data,
+			&rec->sg_plaintext_num_elem,
+			&rec->sg_plaintext_size);
+
 		list_del(&rec->list);
 		kfree(rec);
 	}
-- 
2.13.6

Re: [PATCH net-next] tls: Fix socket mem accounting error under async encryption

[David Miller]

From: Vakul Garg <vakul.garg@nxp.com>
Date: Tue, 25 Sep 2018 16:26:17 +0530

> Current async encryption implementation sometimes showed up socket
> memory accounting error during socket close. This results in kernel
> warning calltrace. The root cause of the problem is that socket var
> sk_forward_alloc gets corrupted due to access in sk_mem_charge()
> and sk_mem_uncharge() being invoked from multiple concurrent contexts
> in multicore processor. The apis sk_mem_charge() and sk_mem_uncharge()
> are called from functions alloc_plaintext_sg(), free_sg() etc. It is
> required that memory accounting apis are called under a socket lock.
> 
> The plaintext sg data sent for encryption is freed using free_sg() in
> tls_encryption_done(). It is wrong to call free_sg() from this function.
> This is because this function may run in irq context. We cannot acquire
> socket lock in this function.
> 
> We remove calling of function free_sg() for plaintext data from
> tls_encryption_done() and defer freeing up of plaintext data to the time
> when the record is picked up from tx_list and transmitted/freed. When
> tls_tx_records() gets called, socket is already locked and thus there is
> no concurrent access problem.
> 
> Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption")
> Signed-off-by: Vakul Garg <vakul.garg@nxp.com>

Applied.

RE: [PATCH net-next] tls: Fix socket mem accounting error under async encryption

[Vakul Garg]

> -----Original Message-----
> From: David Miller <davem@davemloft.net>
> Sent: Tuesday, September 25, 2018 11:14 PM
> To: Vakul Garg <vakul.garg@nxp.com>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com; doronrk@fb.com
> Subject: Re: [PATCH net-next] tls: Fix socket mem accounting error under
> async encryption
> 
> From: Vakul Garg <vakul.garg@nxp.com>
> Date: Tue, 25 Sep 2018 16:26:17 +0530
> 
> > Current async encryption implementation sometimes showed up socket
> > memory accounting error during socket close. This results in kernel
> > warning calltrace. The root cause of the problem is that socket var
> > sk_forward_alloc gets corrupted due to access in sk_mem_charge() and
> > sk_mem_uncharge() being invoked from multiple concurrent contexts in
> > multicore processor. The apis sk_mem_charge() and sk_mem_uncharge()
> > are called from functions alloc_plaintext_sg(), free_sg() etc. It is
> > required that memory accounting apis are called under a socket lock.
> >
> > The plaintext sg data sent for encryption is freed using free_sg() in
> > tls_encryption_done(). It is wrong to call free_sg() from this function.
> > This is because this function may run in irq context. We cannot
> > acquire socket lock in this function.
> >
> > We remove calling of function free_sg() for plaintext data from
> > tls_encryption_done() and defer freeing up of plaintext data to the
> > time when the record is picked up from tx_list and transmitted/freed.
> > When
> > tls_tx_records() gets called, socket is already locked and thus there
> > is no concurrent access problem.
> >
> > Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption")
> > Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
> 
> Applied.
 
I don't find this patch and one other ("tls: Fixed a memory leak during socket close")
in linux-net-next. Could you please kindly check? Regards.

Re: [PATCH net-next] tls: Fix socket mem accounting error under async encryption

[David Miller]

From: Vakul Garg <vakul.garg@nxp.com>
Date: Wed, 26 Sep 2018 01:54:25 +0000

> I don't find this patch and one other ("tls: Fixed a memory leak
> during socket close") in linux-net-next. Could you please kindly
> check? Regards.

After applying I didn't push out and instead I started a test build,
closed my laptop, did a lot of other things and just came back to
finish the build.

It'll show up momentarily.

Thanks for your patience.

RE: [PATCH net-next] tls: Fix socket mem accounting error under async encryption

[Vakul Garg]

> -----Original Message-----
> From: David Miller <davem@davemloft.net>
> Sent: Wednesday, September 26, 2018 9:10 AM
> To: Vakul Garg <vakul.garg@nxp.com>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com; doronrk@fb.com
> Subject: Re: [PATCH net-next] tls: Fix socket mem accounting error under
> async encryption
> 
> From: Vakul Garg <vakul.garg@nxp.com>
> Date: Wed, 26 Sep 2018 01:54:25 +0000
> 
> > I don't find this patch and one other ("tls: Fixed a memory leak
> > during socket close") in linux-net-next. Could you please kindly
> > check? Regards.
> 
> After applying I didn't push out and instead I started a test build, closed my
> laptop, did a lot of other things and just came back to finish the build.
> 
> It'll show up momentarily.
> 
> Thanks for your patience.
 
Thanks for explaining the workflow.

BTW, I noticed following build failure.
It gets resolved after reverting d6ab93364734.

  CC [M]  drivers/net/phy/marvell.o
drivers/net/phy/marvell.c: In function 'm88e1121_config_aneg':
drivers/net/phy/marvell.c:468:25: error: 'autoneg' undeclared (first use in this function); did you mean 'put_net'?
  if (phydev->autoneg != autoneg || changed) {
                         ^~~~~~~
                         put_net
drivers/net/phy/marvell.c:468:25: note: each undeclared identifier is reported only once for each function it appears in

Re: [PATCH net-next] tls: Fix socket mem accounting error under async encryption

[David Miller]

From: Vakul Garg <vakul.garg@nxp.com>
Date: Wed, 26 Sep 2018 04:19:25 +0000

> BTW, I noticed following build failure.
> It gets resolved after reverting d6ab93364734.
> 
>   CC [M]  drivers/net/phy/marvell.o
> drivers/net/phy/marvell.c: In function 'm88e1121_config_aneg':
> drivers/net/phy/marvell.c:468:25: error: 'autoneg' undeclared (first use in this function); did you mean 'put_net'?
>   if (phydev->autoneg != autoneg || changed) {
>                          ^~~~~~~
>                          put_net
> drivers/net/phy/marvell.c:468:25: note: each undeclared identifier is reported only once for each function it appears in
> 

Thanks, I just fixed it as follows below.

Please CC: the patch author when you report build failures like this
in the future.

====================
>From 4b1bd69769454175268908f50b32f1cbfee5bb83 Mon Sep 17 00:00:00 2001
From: "David S. Miller" <davem@davemloft.net>
Date: Tue, 25 Sep 2018 22:41:31 -0700
Subject: [PATCH] net: phy: marvell: Fix build.

Local variable 'autoneg' doesn't even exist:

drivers/net/phy/marvell.c: In function 'm88e1121_config_aneg':
drivers/net/phy/marvell.c:468:25: error: 'autoneg' undeclared (first use in this function); did you mean 'put_net'?
  if (phydev->autoneg != autoneg || changed) {
                         ^~~~~~~

Fixes: d6ab93364734 ("net: phy: marvell: Avoid unnecessary soft reset")
Reported-by:Vakul Garg <vakul.garg@nxp.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 drivers/net/phy/marvell.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index b55a7376bfdc..24fc4a73c300 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -465,7 +465,7 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
 	if (err < 0)
 		return err;
 
-	if (phydev->autoneg != autoneg || changed) {
+	if (phydev->autoneg != AUTONEG_ENABLE || changed) {
 		/* A software reset is used to ensure a "commit" of the
 		 * changes is done.
 		 */
-- 
2.17.1