On 2019/10/22 9:59, zhengbin (A) wrote: > On 2019/10/21 21:06, Hannes Reinecke wrote: >> On 10/21/19 3:49 AM, zhengbin (A) wrote: >>> On 2019/10/18 21:43, Martin K. Petersen wrote: >>>> Hannes, >>>> >>>>> The one thing which I patently don't like is the ambivalence between >>>>> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ >>>>> of them is set? IE what would be the correct way of action if >>>>> DRIVER_SENSE is not set, but we have a valid sense code? Or the other >>>>> way around? >>>> I agree, it's a mess. >>>> >>>> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest >>>> and most arcane code in SCSI) >>>> >>>>> But more important, from a quick glance not all drivers set the >>>>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is >>>>> never evaluated after this patchset. >>>> And yet we appear to have several code paths where sense evaluation is >>>> contingent on DRIVER_SENSE. So no matter what, behavior might >>>> change if we enforce consistent semantics. *sigh* >>> So what should we do to prevent unit-value access of sshdr? >>> >> Where do you see it? >> >From my reading, __scsi_execute() is clearing sshdr by way of >> >> __scsi_execute() >> -> scsi_normalize_sense() >> -> memset(sshdr) > __scsi_execute > > req = blk_get_request(sdev->request_queue, > data_direction == DMA_TO_DEVICE ? > REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT); > if (IS_ERR(req)) > return ret; -->just return > rq = scsi_req(req); > > if (bufflen && blk_rq_map_kern(sdev->request_queue, req, > buffer, bufflen, GFP_NOIO)) > goto out; -->just goto out
may be we should init sshdr in __scsi_execute? which is the simplest way, and do not lose anyone. If we init sshdr in the callers, maybe we will lose some function. + /* + * Zero-initialize sshdr for those callers that check the *sshdr + * contents even if no sense data is available. + */ + if (sshdr) + memset(sshdr, 0, sizeof(struct scsi_sense_hdr)); + > >> Cheers, >> >> Hannes
