Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 7 Oct 2019 13:22:15 +0200
From:   Sabrina Dubroca <sd@queasysnail.net>
To:     Taehee Yoo <ap420073@gmail.com>
Cc:     davem@davemloft.net, netdev@vger.kernel.org,
        linux-wireless@vger.kernel.org, jakub.kicinski@netronome.com,
        johannes@sipsolutions.net, j.vosburgh@gmail.com, vfalico@gmail.com,
        andy@greyhouse.net, jiri@resnulli.us, roopa@cumulusnetworks.com,
        saeedm@mellanox.com, manishc@marvell.com, rahulv@marvell.com,
        kys@microsoft.com, haiyangz@microsoft.com,
        stephen@networkplumber.org, sashal@kernel.org, hare@suse.de,
        varun@chelsio.com, ubraun@linux.ibm.com, kgraul@linux.ibm.com,
        jay.vosburgh@canonical.com, schuffelen@google.com, bjorn@mork.no
Subject: Re: [PATCH net v4 12/12] virt_wifi: fix refcnt leak in module exit
 routine
Message-ID: <20191007112215.GA1288400@bistromath.localdomain>
References: <20190928164843.31800-1-ap420073@gmail.com>
 <20190928164843.31800-13-ap420073@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20190928164843.31800-13-ap420073@gmail.com>
User-Agent: Mutt/1.12.1 (2019-06-15)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

2019-09-28, 16:48:43 +0000, Taehee Yoo wrote:
> virt_wifi_newlink() calls netdev_upper_dev_link() and it internally
> holds reference count of lower interface.
> 
> Current code does not release a reference count of the lower interface
> when the lower interface is being deleted.
> So, reference count leaks occur.
> 
> Test commands:
>     ip link add dummy0 type dummy
>     ip link add vw1 link dummy0 type virt_wifi

There should also be "ip link del dummy0" in this reproducer, right?

[...]

> @@ -598,14 +634,24 @@ static int __init virt_wifi_init_module(void)
>  	/* Guaranteed to be locallly-administered and not multicast. */
>  	eth_random_addr(fake_router_bssid);
>  
> +	err = register_netdevice_notifier(&virt_wifi_notifier);
> +	if (err)
> +		return err;
> +

Here err is 0.

>  	common_wiphy = virt_wifi_make_wiphy();
>  	if (!common_wiphy)
> -		return -ENOMEM;
> +		goto notifier;

err is still 0 when we jump...

>  	err = rtnl_link_register(&virt_wifi_link_ops);
>  	if (err)
> -		virt_wifi_destroy_wiphy(common_wiphy);
> +		goto destroy_wiphy;
>  
> +	return 0;
> +
> +destroy_wiphy:
> +	virt_wifi_destroy_wiphy(common_wiphy);
> +notifier:
> +	unregister_netdevice_notifier(&virt_wifi_notifier);
>  	return err;
>  }

... so now we return 0 on failure. Can you add an "err = -ENOMEM"
before "common_wiphy = ..."?

Thanks.

-- 
Sabrina