netdev - RE: [PATCH net v3] driver: veth: Fix one possbile memleak when fail to register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <002101d2c40f$958a7b80$c09f7280$@foxmail.com>
Date:   Wed, 3 May 2017 21:17:14 +0800
From:   "Gao Feng" <gfree.wind@...mail.com>
To:     "'Xin Long'" <lucien.xin@...il.com>
Cc:     "'Gao Feng'" <gfree.wind@....163.com>,
        "'davem'" <davem@...emloft.net>, <jarod@...hat.com>,
        "'Stephen Hemminger'" <stephen@...workplumber.org>,
        <dsa@...ulusnetworks.com>, "'network dev'" <netdev@...r.kernel.org>
Subject: RE: [PATCH net v3] driver: veth: Fix one possbile memleak when fail to register_netdevice

> From: Xin Long [mailto:lucien.xin@...il.com]
> Sent: Wednesday, May 3, 2017 7:26 PM
> On Wed, May 3, 2017 at 2:37 PM, Gao Feng <gfree.wind@...mail.com> wrote:
> >> From: Xin Long [mailto:lucien.xin@...il.com]
> >> Sent: Wednesday, May 3, 2017 1:38 PM
> >> On Wed, May 3, 2017 at 10:07 AM, Gao Feng <gfree.wind@...mail.com>
> >> wrote:
> >> >> From: netdev-owner@...r.kernel.org
> >> >> [mailto:netdev-owner@...r.kernel.org]
> >> >> On Behalf Of Xin Long
> >> >> Sent: Wednesday, May 3, 2017 12:59 AM On Tue, May 2, 2017 at 7:03
> >> >> PM, Gao Feng <gfree.wind@....163.com> wrote:
> >> >> >> From: Xin Long [mailto:lucien.xin@...il.com]
> >> >> >> Sent: Tuesday, May 2, 2017 3:56 PM On Sat, Apr 29, 2017 at
> >> >> >> 11:51 AM,  <gfree.wind@...mail.com> wrote:
> >> >> >> > From: Gao Feng <gfree.wind@...mail.com>
> > [...]
> >> > The fix you mentioned change the original logic.
> >> > The dev->vstats is freed in advance in the ndo_uninit, not destructor.
> >> > It may break the backward.
> >> Sorry, I didn't get your "backward"
> >> I can't see there will be any problem caused by it.
> >> can you say this patch also break the 'backward' ?
> >> https://patchwork.ozlabs.org/patch/748964/
> >>
> >> It's really weird to do dev->reg_state check in ndo_unint ndo_unint
> >> is supposed to free the memory alloced in ndo_init.
> >>
> >
> > I am not sure if it would break the backward, so I said it MAY break.
> > I assumed there may be someone would access the dev->vstats after
> > ndo_uninit, because current veth driver free the mem in the destructor.
> > I selected this approach because I don't want to bring new bugs during fix
> bug.
> >
> > If you're sure it is safe to free dev->vstats in ndo_uninit, I would like to
> update it.
> yes, stats are accessed in .ndo_start_xmit waited by synchronize_net() and
> .ndo_get_stats64 protected by rtnl_lock().

Thanks, I would update the series later with your advice.
I need to wait for a while to collect more comments.
 
> >
> > BTW there are too many drivers which have possible memleak.
> > You could find the list by
> https://www.mail-archive.com/netdev@vger.kernel.org/msg166629.html.
> ah, cool.
> I'm not sure about other dev's stuff, have to check them for sure later.

Expect and thanks your reviews:)

Best Regards
Feng

> 
> >
> > Some drivers allocate the resources in ndo_init, free some in ndo_uninit and
> free left in destructor.
> > I think there are some reasons.
> > We could not move all free in the ndo_uninit from destructor. What's your
> opinion?
> >
> > Best Regards
> > Feng
> >
> >
> >