Date:   Wed, 1 Dec 2021 09:55:02 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Yang Yingliang <yangyingliang@huawei.com>
Cc:     Pavel Skripkin <paskripkin@gmail.com>,
        linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev,
        gregkh@linuxfoundation.org
Subject: Re: [PATCH -next] staging: rtl8192e: rtllib_module: fix missing
 free_netdev() on error in alloc_rtllib()
Message-ID: <20211201065502.GC9522@kadam>
References: <20211130034007.779351-1-yangyingliang@huawei.com>
 <851b57b9-79ac-e7a1-a61a-6f36a5d9d77a@gmail.com>
 <8f1c4e5a-663f-e5f4-a98e-b72ee3a4f365@huawei.com>
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <8f1c4e5a-663f-e5f4-a98e-b72ee3a4f365@huawei.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?RlAlhxf+5RbF5SxcU4a1mM3qn/vgAbfPJfA35LcTTI5lD3DoseiguKM41J?=
 =?iso-8859-1?Q?ghyIfOfVt1U7mX4UhGtqFDsEJYjy/n9ANK39rO6OnBUz7mrgsOW8+QTTLd?=
 =?iso-8859-1?Q?902X1MtaEvvtvNrlU30ip0HMMa4ZCb7B7ajVsob3YwKEZ2iDxzrXBtCbUE?=
 =?iso-8859-1?Q?YC+DyhzRzhtO7i5D+nAASmxw9LhUMT4DTyZICpIAJKfYo9Y21tzapwSDYU?=
 =?iso-8859-1?Q?4+a5fQlDo/zywAZKzfcyXv/OBnRk+Jm6Wr0lyrD1vFfgVPqnzd4ey+qnET?=
 =?iso-8859-1?Q?CNV+bsHWIfCJ9YCiuA0tRFvL5xGlSTAuLFO49K0Vt3fvRVLnr8Y/4HO8mA?=
 =?iso-8859-1?Q?emlKgZiLBWNIqcxmp+UiA+fy/2z/yQtQuRTt8FIuah5lRQIzRk6mpnnWuE?=
 =?iso-8859-1?Q?Uzf1TWsYw1kIdwvqTU7N3NFbyEJ/KBcdO/PT4J8pWZpu/atDMRsZQgmZcB?=
 =?iso-8859-1?Q?t71+7zyoZ8HMMHqaOcXdLGMdmGbfRlmDPaj0FUuZQ/jeeMHlTiCaIOW3jm?=
 =?iso-8859-1?Q?8/sCo2Y2oKVVXjNE2j4Rt55IOp2BIfo6TZxoyRbRJ29PYSqZn/HuONMZFt?=
 =?iso-8859-1?Q?QYzIrSUBtcJPkmNmmanFCao2+c/4VAPdQAvtz0XmSf7E1A259iMSLJ5AeB?=
 =?iso-8859-1?Q?j4TLsapJlC/FvXmTzMNwHDpcEgpgN9+ZZs1cGsUYIzvnE/IN1pyXaOTNK4?=
 =?iso-8859-1?Q?oZJ264z0EdzzjaK26Q2txvS1prZK87h9LxnPbCYJxzr/KKR2FlCTJDM5OD?=
 =?iso-8859-1?Q?iXJAeM1PVcfRO0FMOemu+cTY8PptQeOVlf9ewS6oHcdzePOsbszkJTFOb+?=
 =?iso-8859-1?Q?HusDCQYI5CtaZzy7yX6A+V8bzC1W+cz06mg6nEEop1n+tqNCdkXTlUBFKC?=
 =?iso-8859-1?Q?LZE4xd2xzI80QWsyXolXSbG35aL/ktbXxQsjRiX+jvQWUsohoBlk4Qs2Kh?=
 =?iso-8859-1?Q?1eUbOCdzqi0CkxqDNVUYwzr9QquY1DXrMkU5cpom/355roggOUdlMLi05S?=
 =?iso-8859-1?Q?j6nakfwlrvjDoHfFeeF3BISp4HOXr/GzycXFzXqVC5qH/xe23C8o0nVYRt?=
 =?iso-8859-1?Q?jChaVsIhQ5bVeKB+yMQSBjnm418uQw9BXI81W60Oc3tzDYCHWMG88pJKHo?=
 =?iso-8859-1?Q?MIgVdtrOQJhgwWoHWlqpFs+PWQ+vILGJfmxDQTkP2gOMNK1yXmE0wWZ+2y?=
 =?iso-8859-1?Q?qO98DJG0Wzf2waxRiZWgFobPhUG0wVpVHjcQSvwHkpxkQ98WDposhGVDS/?=
 =?iso-8859-1?Q?FNPaXlvLkSLgdZGQWQyG4E/XsNroF6UY7S3PVi+QatwQRWcPIeJcdqtHVV?=
 =?iso-8859-1?Q?uLw7bLfY1Vtu95SQwvhEPcMvieO00bDLl/XGUpwRMG9BAOdoR/Bt4Jr8kv?=
 =?iso-8859-1?Q?752Jv73Ea3YWRk6bjYB6GeQbzt9j2yCoYRTZHNvB2z8J68So6TSgIhAfWw?=
 =?iso-8859-1?Q?yjLvwInxL1h00l3jZYmYRewFJanXZSrgtqCLL4XCT1I2ljUXU0322MAgyq?=
 =?iso-8859-1?Q?Ii0MEWlcQCuHmR3vymxMy3lZYEZTkNe9zKrUzGDPoBShLh3+zarr6vMZHG?=
 =?iso-8859-1?Q?xiRaBSJs7a+skMdiB+a9VtKUxIz9pnRWucvOlPz8hAsnLmtdzwZjk8GRxc?=
 =?iso-8859-1?Q?aK5ZVELCKRkqZfGSUdARpKpTyPQNNSU3+aDHo8rHvLhWpE1iZt0ZIExGtD?=
 =?iso-8859-1?Q?411aq/CPfu/S2kkGYrtX6cwDVVdA6XkDH3EEDd9i1Ns7zpp1g0hacD961K?=
 =?iso-8859-1?Q?UpKbAJPrlJ4juoVrUnXqyAS3M=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d656136d-985a-4033-3ef0-08d9b4978bb7
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Dec 2021 06:55:22.8478
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: z8s+kFryNM63qOq7z4rLfy/KK31C480VO52alk5mef2W4+pYl4zNQYPOVMADqmago61Tuv0HkLo3UD0rHZtwerJZSIyer6pMbOjfJHnt8r8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR1001MB2254
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10184 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0 adultscore=0
 bulkscore=0 malwarescore=0 mlxlogscore=999 mlxscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112010039
X-Proofpoint-ORIG-GUID: gu6HF-tCg2NdV5NmE3PcwPA5FdB8HmI6
X-Proofpoint-GUID: gu6HF-tCg2NdV5NmE3PcwPA5FdB8HmI6
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Dec 01, 2021 at 10:41:41AM +0800, Yang Yingliang wrote:
> Hi,
> 
> On 2021/12/1 2:57, Pavel Skripkin wrote:
> > On 11/30/21 06:40, Yang Yingliang wrote:
> > > Add the missing free_netdev() before return from alloc_rtllib()
> > > in the error handling case.
> > > 
> > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> > > ---
> > > ? drivers/staging/rtl8192e/rtllib_module.c | 2 +-
> > > ? 1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/rtl8192e/rtllib_module.c
> > > b/drivers/staging/rtl8192e/rtllib_module.c
> > > index 64d9feee1f39..18d898714c5c 100644
> > > --- a/drivers/staging/rtl8192e/rtllib_module.c
> > > +++ b/drivers/staging/rtl8192e/rtllib_module.c
> > > @@ -125,7 +125,7 @@ struct net_device *alloc_rtllib(int sizeof_priv)
> > > ? ????? ieee->pHTInfo = kzalloc(sizeof(struct rt_hi_throughput),
> > > GFP_KERNEL);
> > > ????? if (!ieee->pHTInfo)
> > > -??????? return NULL;
> > > +??????? goto failed;
> > > ? ????? HTUpdateDefaultSetting(ieee);
> > > ????? HTInitializeHTInfo(ieee);
> > > 
> > 
> > Good catch!
> > 
> > There are 2 more possible leaks, tho. rtllib_networks_allocate() and
> > rtllib_softmac_init() should be unwinded too.
> The error path of rtllib_networks_allocate()? won't leak the dev.

You've misunderstood what Pavel is saying.  He's saying that we need to
call rtllib_networks_free() as well as free_netdev().

This code has a "goto failed" and that means it is either going to do
nothing or do everything.  It is a bad style of error handling and it
often has bugs like this.  The best way to write error handling is to
use Free the Last Thing style.

int my_alloc_function()
{
	a = alloc();
	if (!a)
		return -ENOMEM;  // <- there is no last thing

	b = alloc();
	if (!b) {
		ret = -ENOMEM;
		goto free_a;  // <- this name says "a" is the last thing
	}

	c = alloc();
	if (!c) {
		ret = -ENOMEM;
		goto free_b;
	}

	return 0;

free_b:
	free(b);
free_a:
	free(a);

	return ret;
}

In this style of error handling you only need to remember the last
successful allocation and the names tell you what the goto does so it
is much easier to check if it's correct.

Then to create a my_free_function() you can just: Copy and paste the
error handling.  Add a free(c).  Delete the labels.

void my_free_function()
{
	free(c);
	free(b);
	free(a);
}

The free function for alloc_rtllib() is free_rtllib() and it looks like
this:

drivers/staging/rtl8192e/rtllib_module.c
   150  void free_rtllib(struct net_device *dev)
   151  {
   152          struct rtllib_device *ieee = (struct rtllib_device *)
   153                                        netdev_priv_rsl(dev);
   154  
   155          kfree(ieee->pHTInfo);
   156          ieee->pHTInfo = NULL;
                ^^^^^^^^^^^^^^^^^^^^^
This line is pointless and should be deleted.

   157          rtllib_softmac_free(ieee);
   158  
   159          lib80211_crypt_info_free(&ieee->crypt_info);
   160  
   161          rtllib_networks_free(ieee);
   162          free_netdev(dev);
   163  }

As you can see this free function calls rtllib_softmac_free(),
lib80211_crypt_info_free() and rtllib_networks_free() so the error
handling in alloc_rtllib() needs to do that as well.  Based on what
I have said, then ideally the error handling for alloc_rtllib() would
look something like this:

        return dev;

free_softmac:
        rtllib_softmac_free(ieee);
        lib80211_crypt_info_free(&ieee->crypt_info);
free_networks:
        rtllib_networks_free(ieee);
free_netdev:
        free_netdev(dev);

        return NULL;

The rtllib_softmac_init() function should really return an int, but it
returns void.  That could be fixed in a separate patch if you want.

regards,
dan carpenter