Re: [RFC PATCH 1/6] bsg: fix kernel panic resulting from missing allocation of a reply-buffer

[Christoph Hellwig <hch@lst.de>]

On Mon, Aug 14, 2017 at 06:32:17PM +0200, Benjamin Block wrote:
> > -	blk_end_request_all(rq, BLK_STS_OK);
> > -
> >  	put_device(job->dev);	/* release reference for the request */
> >
> >  	kfree(job->request_payload.sg_list);
> >  	kfree(job->reply_payload.sg_list);
> > -	kfree(job);
> > +	blk_end_request_all(rq, BLK_STS_OK);
> 
> What is the reason for moving that last line? Just wondering whether
> that might change the behavior somehow, although it doesn't look like it
> from the code.

The job is now allocated as part of the request, so we must fre
it last.  The only change in behavior is that the reference gets dropped
a bit earlier, but once ownership is handed to the block layer
it's not needed, as are the memory allocations for the S/G lists.

> > +{
> > +	struct bsg_job *job = blk_mq_rq_to_pdu(req);
> > +
> > +	memset(job, 0, sizeof(*job));
> > +	job->req = req;
> > +	job->request = job->sreq.cmd;
> 
> That doesn't work with bsg.c if the request submitted by the user is
> bigger than BLK_MAX_CDB. There is code in blk_fill_sgv4_hdr_rq() that
> will reassign the req->cmd point in that case to something else.
> 
> This is maybe wrong in the same vein as my Patch 1 is. If not for the
> legacy code in bsg.c, setting this here, will miss changes to that
> pointer between request-allocation and job-submission.
> 
> So maybe just move this to bsg_create_job().

Yes, this should be in  indeed.

> 
> > +	job->dd_data = job + 1;
> > +	job->reply = job->sreq.sense = kzalloc(job->reply_len, gfp);
> 
> job->reply_len will be 0 here, won't it? You probably have to pull the
> "job->reply_len = SCSI_SENSE_BUFFERSIZE" here from bsg_create_job().

True.