netdev - Re: [RFC] about net: Fix inconsistent teardown and release of private netdev state.

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: <1503114001.14953.8.camel@edumazet-glaptop3.roam.corp.google.com>
Date:   Fri, 18 Aug 2017 20:40:01 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org
Subject: Re: [RFC] about net: Fix inconsistent teardown and release of
 private netdev state.

On Fri, 2017-08-18 at 15:58 -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@...il.com>
> Date: Fri, 18 Aug 2017 06:13:49 -0700
> 
> > On Thu, 2017-08-17 at 22:21 -0700, David Miller wrote:
> >> From: Eric Dumazet <eric.dumazet@...il.com>
> >> Date: Thu, 17 Aug 2017 15:30:40 -0700
> >> 
> >> > So we do not really know if we need to clean up or not.
> >> 
> >> We always know, the answer is that whenever register_netdev() fails we
> >> never need to perform any cleanup which is done by priv_destructor.
> >> 
> >> > Any idea how to fix the issue ?
> >> 
> >> Your patch is exactly how we should fix this, but without the comment.
> >> The logic is straightforward.
> >> 
> >> If register_netdevice() fails any resources handled by priv_destructor
> >> are cleaned up, it is guaranteed.
> > 
> > Not in current code.
> > 
> > There are some failures which do a "goto out;" 
> > 
> > out:
> > 	return ret;
> > 
> > 
> > In these cases, priv_destructor is not called.
> > 
> > So we need multiple fixes I think :/
> 
> I don't think so.
> 
> The cases that "goto out;" in register_netdevce() are those that
> execute before ->ndo_init() succeeds.
> 
> Only if ->ndo_succeeds() runs successfully should ->priv_destructor()
> need execute.
> 
> So everything is fine as far as I can see.

Let look at tun->pcpu_stats, for example.

It is allocated at line 1831, before the register_netdevice()

drivers/net/tun.c does not provide ndo_init()