linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>,
	Phillip Potter <phil@philpotter.co.uk>,
	Martin Kaiser <martin@kaiser.cx>,
	Michael Straube <straube.linux@gmail.com>,
	Pavel Skripkin <paskripkin@gmail.com>,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
	Aakash Hemadri <aakashhemadri123@gmail.com>
Subject: Re: [PATCH v5] staging: r8188eu: Remove _enter/_exit_critical_mutex()
Date: Thu, 2 Sep 2021 11:32:07 +0200	[thread overview]
Message-ID: <YTCaF5Ure2tjfbZB@kroah.com> (raw)
In-Reply-To: <20210828113656.6963-1-fmdefrancesco@gmail.com>

On Sat, Aug 28, 2021 at 01:36:56PM +0200, Fabio M. De Francesco wrote:
> Remove _enter_critical_mutex() and _exit_critical_mutex(). They are
> unnecessary wrappers, respectively to mutex_lock_interruptible() and
> to mutex_unlock(). They also have an odd interface that takes an unused
> argument named pirqL of type unsigned long.
> The original code enters the critical section if the mutex API is
> interrupted while waiting to acquire the lock; therefore it could lead
> to a race condition. Use mutex_lock() because it is uninterruptible and
> so avoid that above-mentioned potential race condition.
> 
> Tested-by: Pavel Skripkin <paskripkin@gmail.com>
> Reviewed-by: Pavel Skripkin <paskripkin@gmail.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
> 
> v5: Fix a typo in the subject line. Reported by Aakash Hemadri.
> 
> v4: Tested and reviewed by Pavel Skripkin. No changes to the code.
> 
> v3: Assume that the original authors don't expect that
> mutex_lock_interruptible() can be really interrupted and then lead to 
> a potential race condition. Furthermore, Greg Kroah-Hartman makes me
> notice that "[] one almost never needs interruptable locks in a driver".
> Therefore, replace the calls to mutex_lock_interruptible() with calls to
> mutex_lock() since the latter is uninterruptible and avoid race
> conditions without the necessity to handle -EINTR errors.

Based on a recent conversation on the linux-usb mailing list, perhaps I
was wrong:
	https://lore.kernel.org/r/20210829015825.GA297712@rowland.harvard.edu

Can you check what happens with your change when you disconnect the
device and these code paths are being called?  That is when you do want
the lock interrupted.

Yes, the logic still seems wrong, but I don't want to see the code now
just lock up entirely with this change as it is a change in how things
work from today.

thanks,

greg k-h

  reply	other threads:[~2021-09-02  9:32 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-28 11:36 [PATCH v5] staging: r8188eu: Remove _enter/_exit_critical_mutex() Fabio M. De Francesco
2021-09-02  9:32 ` Greg Kroah-Hartman [this message]
2021-09-03 15:09   ` Pavel Skripkin
2021-09-03 19:11     ` Pavel Skripkin
2021-09-04  8:15   ` Fabio M. De Francesco

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YTCaF5Ure2tjfbZB@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=Larry.Finger@lwfinger.net \
    --cc=aakashhemadri123@gmail.com \
    --cc=fmdefrancesco@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=martin@kaiser.cx \
    --cc=paskripkin@gmail.com \
    --cc=phil@philpotter.co.uk \
    --cc=straube.linux@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).