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:	Tue, 5 May 2015 16:20:25 +0000
From:	Jose Rivera <German.Rivera@...escale.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
CC:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"arnd@...db.de" <arnd@...db.de>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Stuart Yoder <stuart.yoder@...escale.com>,
	"bhupesh.sharma@...escale.com" <bhupesh.sharma@...escale.com>,
	"agraf@...e.de" <agraf@...e.de>,
	"bhamciu1@...escale.com" <bhamciu1@...escale.com>,
	"nir.erez@...escale.com" <nir.erez@...escale.com>,
	"itai.katz@...escale.com" <itai.katz@...escale.com>,
	Scott Wood <scottwood@...escale.com>,
	"R89243@...escale.com" <R89243@...escale.com>,
	Richard Schmitt <richard.schmitt@...escale.com>
Subject: RE: [PATCH 6/7] staging: fsl-mc: Add locking to serialize
 mc_send_command() calls



> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@...cle.com]
> Sent: Thursday, April 30, 2015 7:59 AM
> To: Rivera Jose-B46482
> Cc: gregkh@...uxfoundation.org; arnd@...db.de;
> devel@...verdev.osuosl.org; linux-kernel@...r.kernel.org; Yoder Stuart-
> B08248; Sharma Bhupesh-B45370; agraf@...e.de; Hamciuc Bogdan-BHAMCIU1;
> Erez Nir-RM30794; katz Itai-RM05202; Wood Scott-B07421; Marginean
> Alexandru-R89243; Schmitt Richard-B43082
> 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.
> 
What would be a reasonable max time for holding a spinlock here?

> >
> > +	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/