Re: [RFC PATCH v2 09/13] ib/core: Enforce PKey security when modifying QPs

["leon-2ukJVAZIZ/Y@public.gmane.org" <leon-2ukJVAZIZ/Y@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]

On Thu, Apr 07, 2016 at 09:02:43PM +0000, Daniel Jurgens wrote:
> On 4/7/2016 11:31 AM, Leon Romanovsky wrote:
> > On Thu, Apr 07, 2016 at 02:33:54AM +0300, Dan Jurgens wrote:
> > 
> >> +	if (sec->qp == sec->qp->real_qp) {
> >> +		/* The caller of this function holds the QP security
> >> +		 * mutex so this list traversal is safe
> >> +		*/
> > 
> > Did the comment below pass checkpatch.pl?
> 
> It did.
> 
> >> +		list_for_each_entry(shared_qp_sec,
> >> +				    &sec->shared_qp_list,
> >> +				    shared_qp_list) {
> > 
> > Is this list always needed to be protected by lock?
> > In general, I prefer to see lock/unlock operations near protected code.
> > Otherwise, it is hard to follow all lock/unlock paths.
> 
> The mutex is required, and I'm not sure how to push it lower safely.
> Consider the situation where a QP is being modified concurrently with an
> open_shared_qp. Unless the lock is held the entire time for each
> operation open_shared_qp could check permission using a ib_qp_security
> struct in a partially modified state, i.e. there could be a mix of new
> and old settings for the port/pkey index/alternate path.  Shared QPs use
> the port and pkey setting for the underlying real QP with the shared qp
> q_security.

Common flow is to do everything in one place:
1. lock
2. check permissions
3. change
4. release lock

Your flow is much broader.
1. lock
2. check permission
3. do work
4. change
5. do work
6. release lock.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]