Return-path: Received: from mleia.com ([178.79.152.223]:52460 "EHLO mail.mleia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934378AbcCJAC7 (ORCPT ); Wed, 9 Mar 2016 19:02:59 -0500 Subject: Re: [PATCH] staging: wilc1000: fix check of kthread_run() return value To: Julian Calaby References: <1457565194-31533-1-git-send-email-vz@mleia.com> <56E0B21D.1010503@mleia.com> Cc: Glen Lee , Greg Kroah-Hartman , Johnny Kim , Austin Shin , Chris Park , Tony Cho , Leo Kim , linux-wireless , "devel@driverdev.osuosl.org" From: Vladimir Zapolskiy Message-ID: <56E0B9AD.3050806@mleia.com> (sfid-20160310_010303_234099_DB75A851) Date: Thu, 10 Mar 2016 02:02:53 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: Hi Julian, On 10.03.2016 01:42, Julian Calaby wrote: > Hi Vladimir, > > On Thu, Mar 10, 2016 at 10:30 AM, Vladimir Zapolskiy wrote: >> Hi Julian, >> >> On 10.03.2016 01:27, Julian Calaby wrote: >>> Hi Vladimir, >>> >>> On Thu, Mar 10, 2016 at 10:13 AM, Vladimir Zapolskiy wrote: >>>> The kthread_run() function returns either a valid task_struct or >>>> ERR_PTR() value, check for NULL is invalid. The change fixes potential >>>> oops, e.g. in OOM situation. >>>> >>>> Signed-off-by: Vladimir Zapolskiy >>>> --- >>>> drivers/staging/wilc1000/linux_wlan.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c >>>> index 54fe9d7..5077c30 100644 >>>> --- a/drivers/staging/wilc1000/linux_wlan.c >>>> +++ b/drivers/staging/wilc1000/linux_wlan.c >>>> @@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct net_device *dev) >>>> PRINT_D(INIT_DBG, "Creating kthread for transmission\n"); >>>> wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev, >>>> "K_TXQ_TASK"); >>>> - if (!wilc->txq_thread) { >>>> + if (IS_ERR(wilc->txq_thread)) { >>>> PRINT_ER("couldn't create TXQ thread\n"); >>>> wilc->close = 0; >>>> - return -ENOBUFS; >>>> + return PTR_ERR(wilc->txq_thread); >>> >>> Are you sure changing the error returned is correct? Do all the >>> callers of wlan_initialize_threads() handle the full range of errors >>> from kthread_run()? >> >> Have you checked the driver? > > I'm making sure you have. It's possible that there's a good reason why > this returns -ENOBUFS I want to know that you've at least considered > that possibility. You have my confirmation, I've checked the call stack before publishing this fix. >> This function is called once on initialization, the check on the upper layer >> has "if (ret < 0) goto exit_badly;" form. > > And practically everything in the chain up to net_device_ops uses the > same error handling scheme so it's probably fine. dev_open() __dev_open() wilc_mac_open() wilc1000_wlan_init() wlan_initialize_threads() Oh, why kernel threads within a driver are init'ed/destroyed on each device up/down state transition? > You should also document this change in the commit message. The change is documented in the commit message, take a look. But I didn't add "I swear it does not break anything" ;) -- With best wishes, Vladimir