linux-kernel - Re: [PATCH 6/7] staging: fsl-mc: Add locking to serialize mc_send

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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 30 Apr 2015 15:59:13 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	"J. German Rivera" <German.Rivera@...escale.com>
Cc:	gregkh@...uxfoundation.org, arnd@...db.de,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
	stuart.yoder@...escale.com, bhupesh.sharma@...escale.com,
	agraf@...e.de, bhamciu1@...escale.com, nir.erez@...escale.com,
	itai.katz@...escale.com, scottwood@...escale.com,
	R89243@...escale.com, richard.schmitt@...escale.com
Subject: Re: [PATCH 6/7] staging: fsl-mc: Add locking to serialize
 mc_send_command() calls

On Tue, Apr 28, 2015 at 12:39:09PM -0500, J. German Rivera wrote:
> @@ -230,15 +235,26 @@ static inline enum mc_cmd_status mc_read_response(struct mc_command __iomem *
>   * @cmd: command to be sent
>   *
>   * Returns '0' on Success; Error code otherwise.
> - *
> - * NOTE: This function cannot be invoked from from atomic contexts.
>   */
>  int mc_send_command(struct fsl_mc_io *mc_io, struct mc_command *cmd)
>  {
> +	int error;
>  	enum mc_cmd_status status;
>  	unsigned long jiffies_until_timeout =
>  	    jiffies + MC_CMD_COMPLETION_TIMEOUT_JIFFIES;

We busy loop while holding a spinlock for half a second.  That seems
bad.

>  
> +	if (preemptible()) {

This is wrong.  If the user asked for spinlocks they should always
get spinlocks.  It shouldn't matter that they are not currently holding
a different lock.

I'm skeptical of this locking anyway.

Also what about if they have PREEMPT disabled?  There aren't any users
for this stuff anyway so it's impossible to review how people are
FSL_MC_IO_ATOMIC_CONTEXT_PORTAL.

Let's wait until there is a user before looking at this.

> -		return mc_status_to_error(status);
> +		error = mc_status_to_error(status);
> +		goto common_exit;
>  	}
>  
> -	return 0;
> +	error = 0;
> +
> +common_exit:

Just name this unlock:.

> +	if (preemptible())
> +		mutex_unlock(&mc_io->mutex);
> +	else
> +		spin_unlock(&mc_io->spinlock);
> +
> +	return error;
>  }

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/