Thread (9 messages) flat view 9 messages, 2 authors, 2018-01-16

Re: [Patch net v3] tun: fix a memory leak for tfile->tx_array

From: Jason Wang <hidden>
Date: 2018-01-16 05:46:44


On 2018年01月16日 03:37, Cong Wang wrote:
quoted hunk ↗ jump to hunk
tfile->tun could be detached before we close the tun fd,
via tun_detach_all(), so it should not be used to check for
tfile->tx_array.

As Jason suggested, we probably have to clean it up
unconditionally both in __tun_deatch() and tun_detach_all(),
but this requires to check if it is initialized or not.
Currently skb_array_cleanup() doesn't have such a check,
so I check it in the caller and introduce a helper function,
it is a bit ugly but we can always improve it in net-next.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Fixes: 1576d9860599 ("tun: switch to use skb array for tx")
Cc: Jason Wang <redacted>
Signed-off-by: Cong Wang <redacted>
---
  drivers/net/tun.c | 15 +++++++++++++--
  1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 4f4a842a1c9c..a8ec589d1359 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -611,6 +611,14 @@ static void tun_queue_purge(struct tun_file *tfile)
  	skb_queue_purge(&tfile->sk.sk_error_queue);
  }
  
+static void tun_cleanup_tx_array(struct tun_file *tfile)
+{
+	if (tfile->tx_array.ring.queue) {
+		skb_array_cleanup(&tfile->tx_array);
+		memset(&tfile->tx_array, 0, sizeof(tfile->tx_array));
+	}
+}
+
  static void __tun_detach(struct tun_file *tfile, bool clean)
  {
  	struct tun_file *ntfile;
@@ -657,8 +665,7 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
  			    tun->dev->reg_state == NETREG_REGISTERED)
  				unregister_netdevice(tun->dev);
  		}
-		if (tun)
-			skb_array_cleanup(&tfile->tx_array);
+		tun_cleanup_tx_array(tfile);
  		sock_put(&tfile->sk);
  	}
  }
@@ -700,11 +707,13 @@ static void tun_detach_all(struct net_device *dev)
  		/* Drop read queue */
  		tun_queue_purge(tfile);
  		sock_put(&tfile->sk);
+		tun_cleanup_tx_array(tfile);
  	}
  	list_for_each_entry_safe(tfile, tmp, &tun->disabled, next) {
  		tun_enable_queue(tfile);
  		tun_queue_purge(tfile);
  		sock_put(&tfile->sk);
+		tun_cleanup_tx_array(tfile);
  	}
  	BUG_ON(tun->numdisabled != 0);
  
@@ -2851,6 +2860,8 @@ static int tun_chr_open(struct inode *inode, struct file * file)
  
  	sock_set_flag(&tfile->sk, SOCK_ZEROCOPY);
  
+	memset(&tfile->tx_array, 0, sizeof(tfile->tx_array));
+
  	return 0;
  }
  
I think then you don't even need the memset trick since we are sure it 
has been implemented?

Thanks
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help