| Message ID | 20180115193729.13220-1-xiyou.wangcong@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v3] tun: fix a memory leak for tfile->tx_array | expand |
On 2018年01月16日 03:37, Cong Wang wrote: > 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 <jasowang@redhat.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > 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
On Mon, Jan 15, 2018 at 9:46 PM, Jason Wang <jasowang@redhat.com> wrote: > > > I think then you don't even need the memset trick since we are sure it has > been implemented? It doesn't look like sk_alloc() zero's the memory of tfile.
On 2018年01月16日 13:49, Cong Wang wrote: > On Mon, Jan 15, 2018 at 9:46 PM, Jason Wang <jasowang@redhat.com> wrote: >> >> I think then you don't even need the memset trick since we are sure it has >> been implemented? > It doesn't look like sk_alloc() zero's the memory of tfile. Typo, for "implemented" I mean "initialized". I mean we can leave __tun_detach() as is, and just add the cleanup to tun_detach_all(). This is because in both cases, we're sure skb array has been initialized before. Thanks
On Mon, Jan 15, 2018 at 10:00 PM, Jason Wang <jasowang@redhat.com> wrote: > I mean we can leave __tun_detach() as is, and just add the cleanup to > tun_detach_all(). This is because in both cases, we're sure skb array has > been initialized before. > Oh, I thought the same before sending v3, but I believe it is easier to understand 'if (tfile->tx_array.ring.queue)' than 'if (tun)', because tx_array only depends on itself rather tfile->tun in this way.
On 2018年01月16日 14:07, Cong Wang wrote: > On Mon, Jan 15, 2018 at 10:00 PM, Jason Wang <jasowang@redhat.com> wrote: >> I mean we can leave __tun_detach() as is, and just add the cleanup to >> tun_detach_all(). This is because in both cases, we're sure skb array has >> been initialized before. >> > Oh, I thought the same before sending v3, but I believe it is easier to > understand 'if (tfile->tx_array.ring.queue)' than 'if (tun)', because tx_array > only depends on itself rather tfile->tun in this way. Maybe just add a comment to explain in __tun_detach(), it avoids memset() anyway. Thanks
On Mon, Jan 15, 2018 at 10:12 PM, Jason Wang <jasowang@redhat.com> wrote: > > > On 2018年01月16日 14:07, Cong Wang wrote: >> >> On Mon, Jan 15, 2018 at 10:00 PM, Jason Wang <jasowang@redhat.com> wrote: >>> >>> I mean we can leave __tun_detach() as is, and just add the cleanup to >>> tun_detach_all(). This is because in both cases, we're sure skb array has >>> been initialized before. >>> >> Oh, I thought the same before sending v3, but I believe it is easier to >> understand 'if (tfile->tx_array.ring.queue)' than 'if (tun)', because >> tx_array >> only depends on itself rather tfile->tun in this way. > > > Maybe just add a comment to explain in __tun_detach(), it avoids memset() > anyway. > But __tun_detach(true) is not a hot path, a memset() doesn't harm anything.
On 2018年01月16日 14:33, Cong Wang wrote: > On Mon, Jan 15, 2018 at 10:12 PM, Jason Wang <jasowang@redhat.com> wrote: >> >> On 2018年01月16日 14:07, Cong Wang wrote: >>> On Mon, Jan 15, 2018 at 10:00 PM, Jason Wang <jasowang@redhat.com> wrote: >>>> I mean we can leave __tun_detach() as is, and just add the cleanup to >>>> tun_detach_all(). This is because in both cases, we're sure skb array has >>>> been initialized before. >>>> >>> Oh, I thought the same before sending v3, but I believe it is easier to >>> understand 'if (tfile->tx_array.ring.queue)' than 'if (tun)', because >>> tx_array >>> only depends on itself rather tfile->tun in this way. >> >> Maybe just add a comment to explain in __tun_detach(), it avoids memset() >> anyway. >> > But __tun_detach(true) is not a hot path, a memset() doesn't harm anything. Yes, but it looks more more like a workaround or trick to me. Thanks
On Mon, Jan 15, 2018 at 10:37 PM, Jason Wang <jasowang@redhat.com> wrote: > > > On 2018年01月16日 14:33, Cong Wang wrote: >> But __tun_detach(true) is not a hot path, a memset() doesn't harm >> anything. > > > Yes, but it looks more more like a workaround or trick to me. > I'd blame skb_array API for this. ;) Ideally, skb_array_cleanup() should take care of everything I put in tun_cleanup_tx_array(). As I mentioned in changelog, we can always improve it in -net-next, so I don't want to bother it for -net.
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; }
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 <jasowang@redhat.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- drivers/net/tun.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)