[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20200609121816.GS2880@minyard.net>
Date: Tue, 9 Jun 2020 07:18:16 -0500
From: Corey Minyard <minyard@....org>
To: wu000273@....edu
Cc: kjlu@....edu, Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
openipmi-developer@...ts.sourceforge.net,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ipmi: code cleanup and prevent potential issue.
On Tue, Jun 09, 2020 at 01:04:10AM -0500, wu000273@....edu wrote:
> From: Qiushi Wu <wu000273@....edu>
>
> All the previous get/put operations against intf->refcount are
> inside the mutex. Thus, put the last kref_put() also inside mutex
> to make sure get/put functions execute in order and prevent the
> potential race condition.
No, this can result in a crash. intf and intf->bmc_reg_mutex will
be freed by intf_free. In fact, every call to kref_put() on intf
better be outside any mutex/lock in intf. If you saw any, that
is a bug, please report that. kref_get() is fine inside the
mutex.
Plus, this is not a race condition. get/put is atomic.
-corey
>
> Signed-off-by: Qiushi Wu <wu000273@....edu>
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index e1b22fe0916c..d34343e34272 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -2583,10 +2583,11 @@ static int __bmc_get_device_id(struct ipmi_smi *intf, struct bmc_device *bmc,
> *guid = bmc->guid;
> }
>
> + kref_put(&intf->refcount, intf_free);
> +
> mutex_unlock(&bmc->dyn_mutex);
> mutex_unlock(&intf->bmc_reg_mutex);
>
> - kref_put(&intf->refcount, intf_free);
> return rv;
> }
>
> --
> 2.17.1
>
Powered by blists - more mailing lists