[ofa-general] Re: sg_reset can trigger a NULL pointer dereference in the SRP initiator

[Roland Dreier]

 > A fix like the one below ?

I think this gets us part of the way, but not quite.

 > --- linux-2.6.30.4/drivers/infiniband/ulp/srp/ib_srp-orig.c	2009-08-03
 > 12:13:11.000000000 +0200
 > +++ linux-2.6.30.4/drivers/infiniband/ulp/srp/ib_srp.c	2009-08-07
 > 10:23:27.000000000 +0200
 > @@ -1371,16 +1371,27 @@ out:
 >  	return -1;
 >  }
 > 
 > +/**
 > + * Look up the struct srp_request that has been associated with the specified
 > + * SCSI command by srp_queuecommand().
 > + *
 > + * Returns 0 upon success and -1 upon failure.
 > + */
 >  static int srp_find_req(struct srp_target_port *target,
 >  			struct scsi_cmnd *scmnd,
 >  			struct srp_request **req)
 >  {
 > -	if (scmnd->host_scribble == (void *) -1L)
 > -		return -1;
 > +	/*
 > +	 * The code below will only work if SRP_RQ_SIZE is a power of two,
 > +	 * so check this first.
 > +	 */
 > +	BUILD_BUG_ON((SRP_RQ_SIZE ^ (SRP_RQ_SIZE - 1))
 > +		     != (SRP_RQ_SIZE | (SRP_RQ_SIZE - 1)));

could this be BUILD_BUG_ON(!is_power_of_2(SRP_RQ_SIZE)) ?
 > 
 > -	*req = &target->req_ring[(long) scmnd->host_scribble];
 > +	*req = &target->req_ring[(long)scmnd->host_scribble
 > +				 & (SRP_RQ_SIZE - 1)];
 > 
 > -	return 0;
 > +	return (*req)->scmnd == scmnd ? 0 : -1;
 >  }
 > 
 >  static int srp_abort(struct scsi_cmnd *scmnd)
 > @@ -1423,8 +1434,15 @@ static int srp_reset_device(struct scsi_
 > 
 >  	if (target->qp_in_error)
 >  		return FAILED;
 > -	if (srp_find_req(target, scmnd, &req))
 > -		return FAILED;
 > +	if (srp_find_req(target, scmnd, &req)) {
 > +		/*
 > +		 * scmnd has not yet been queued -- queue it now. This can
 > +		 * happen e.g. when a SG_SCSI_RESET ioctl has been issued.
 > +		 */
 > +		if (srp_queuecommand(scmnd, scmnd->scsi_done)
 > +		    || srp_find_req(target, scmnd, &req))
 > +			return FAILED;

I don't think we can just pass the command to srp_queuecommand() here.
For one thing queuecommand requires some locking, and second, we don't
actually want to queue the command -- in fact I'm not sure it is set up
properly with an opcode etc to execute the command.

What I think needs to happen is we need to allocate a request for the
command the same way srp_queuecommand() does, and in fact maybe that
code could be factored out to avoid duplication.

 -R .