netdev - RE: [PATCH net-next, 2/4] hv_netvsc: Fix error handling in netvsc

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]

Date:   Mon, 4 Nov 2019 15:08:43 +0000
From:   Haiyang Zhang <haiyangz@...rosoft.com>
To:     Markus Elfring <Markus.Elfring@....de>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        KY Srinivasan <kys@...rosoft.com>,
        Olaf Hering <olaf@...fle.de>, Sasha Levin <sashal@...nel.org>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        vkuznets <vkuznets@...hat.com>
Subject: RE: [PATCH net-next, 2/4] hv_netvsc: Fix error handling in
 netvsc_attach()



> -----Original Message-----
> From: Markus Elfring <Markus.Elfring@....de>
> Sent: Friday, November 1, 2019 4:43 PM
> To: Haiyang Zhang <haiyangz@...rosoft.com>; linux-
> hyperv@...r.kernel.org; netdev@...r.kernel.org
> Cc: kernel-janitors@...r.kernel.org; linux-kernel@...r.kernel.org; David S.
> Miller <davem@...emloft.net>; KY Srinivasan <kys@...rosoft.com>; Olaf
> Hering <olaf@...fle.de>; Sasha Levin <sashal@...nel.org>; Stephen
> Hemminger <sthemmin@...rosoft.com>; vkuznets <vkuznets@...hat.com>
> Subject: Re: [PATCH net-next, 2/4] hv_netvsc: Fix error handling in
> netvsc_attach()
> 
> > If rndis_filter_open() fails, we need to remove the rndis device
> > created in earlier steps, before returning an error code. Otherwise,
> > the retry of
> > netvsc_attach() from its callers will fail and hang.
> 
> How do you think about to choose a more “imperative mood” for your
> change description?
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.k
> ernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%
> 2Ftree%2FDocumentation%2Fprocess%2Fsubmitting-
> patches.rst%3Fid%3D0dbe6cb8f7e05bc9611602ef45980a6c57b245a3%23n151
> &amp;data=02%7C01%7Chaiyangz%40microsoft.com%7C162aa016f45e4293c
> abb08d75f0c0fee%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637
> 082377796295159&amp;sdata=ytjxGYTPI2D4BoNbslKPvBbsfGUEM7hXj1YAiG
> hn8Ik%3D&amp;reserved=0
Agreed. Thanks.


> 
> 
> …
> > +++ b/drivers/net/hyperv/netvsc_drv.c
> > @@ -982,7 +982,7 @@ static int netvsc_attach(struct net_device *ndev,
> >  	if (netif_running(ndev)) {
> >  		ret = rndis_filter_open(nvdev);
> >  		if (ret)
> > -			return ret;
> > +			goto err;
> >
> >  		rdev = nvdev->extension;
> >  		if (!rdev->link_state)
> …
> 
> I would prefer to specify the completed exception handling (addition of two
> function calls) by a compound statement in the shown if branch directly.
> 
> If you would insist to use a goto statement, I find an other label more
> appropriate according to the Linux coding style.

I will have more patches that make multiple entry points of error clean up 
steps -- so I used goto instead of putting the functions in each if-branch.

I will name the labels more meaningfully.

Thanks,
- Haiyang