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: <000c01d2c3b2$0925e880$1b71b980$@foxmail.com>
Date:   Wed, 3 May 2017 10:07:36 +0800
From:   "Gao Feng" <gfree.wind@...mail.com>
To:     "'Xin Long'" <lucien.xin@...il.com>,
        "'Gao Feng'" <gfree.wind@....163.com>
Cc:     "'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: 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>
> > [...]
> >> > -static void veth_dev_free(struct net_device *dev)
> >> > +static void veth_destructor_free(struct net_device *dev)
> >> >  {
> >> >         free_percpu(dev->vstats);
> >> > +}
> >> not sure why you needed to add this function.
> >> to use free_percpu() directly may be clearer.
> >
> > Because both of ndo_uninit and destructor need to perform same free
> statements.
> > It is good at maintain the codes with the common function.
> >>
> >> > +
> >> > +static void veth_dev_uninit(struct net_device *dev) {
> >> call free_percpu() here, no need to check dev->reg_state.
> >> free_percpu will just return if dev->vstats is NULL.
> >
> > It would break the original design if don't check the reg_state.
> > The original logic is that free the resources in the destructor, not in ndo_init.
> I got what you're doing now, can you pls try to fix this with:
> 
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -219,10 +219,9 @@ static int veth_dev_init(struct net_device *dev)
>         return 0;
>  }
> 
> -static void veth_dev_free(struct net_device *dev)
> +static void veth_dev_uninit(struct net_device *dev)
>  {
>         free_percpu(dev->vstats);
> -       free_netdev(dev);
>  }
> 
>  #ifdef CONFIG_NET_POLL_CONTROLLER
> @@ -279,6 +278,7 @@ static void veth_set_rx_headroom(struct net_device
> *dev, int new_hr)
> 
>  static const struct net_device_ops veth_netdev_ops = {
>         .ndo_init            = veth_dev_init,
> +       .ndo_uninit          = veth_dev_uninit,
>         .ndo_open            = veth_open,
>         .ndo_stop            = veth_close,
>         .ndo_start_xmit      = veth_xmit,
> @@ -317,7 +317,7 @@ static void veth_setup(struct net_device *dev)
>                                NETIF_F_HW_VLAN_STAG_TX |
>                                NETIF_F_HW_VLAN_CTAG_RX |
>                                NETIF_F_HW_VLAN_STAG_RX);
> -       dev->destructor = veth_dev_free;
> +       dev->destructor = free_netdev;
>         dev->max_mtu = ETH_MAX_MTU;
> 
>         dev->hw_features = VETH_FEATURES;
> 
> 
> just as what other virtual nic drivers do (vxlan, geneve, macsec, bridge ....)
> 

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.

Regards
Feng