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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADvbK_dqU7wDQoU2nHBwPkQjzSuiVGXStYBD9iZSvz=j8SGt-w@mail.gmail.com>
Date:   Wed, 3 May 2017 19:25:38 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Gao Feng <gfree.wind@...mail.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

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().


>
> 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.

>
> 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
>
>
>