linux-kernel - Re: [PATCH 1/2] scsi: Do not attach VPD to devices that don't support it

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]

Message-ID: <CAKgT0Ue-eGnKQ_mydter8qtbUWVuNd+HMziHs9QT+g9g8hrvAw@mail.gmail.com>
Date:	Thu, 21 Jan 2016 09:05:42 -0800
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Hannes Reinecke <hare@...e.de>
Cc:	Alexander Duyck <aduyck@...antis.com>,
	James Bottomley <James.Bottomley@...senpartnership.com>,
	linux-scsi@...r.kernel.org, martin.petersen@...cle.com,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Shane M. Seymour" <shane.seymour@....com>, jthumshirn@...e.de
Subject: Re: [PATCH 1/2] scsi: Do not attach VPD to devices that don't support it

On Wed, Jan 20, 2016 at 11:37 PM, Hannes Reinecke <hare@...e.de> wrote:
> On 01/21/2016 07:35 AM, Alexander Duyck wrote:
>> The patch "scsi: rescan VPD attributes" introduced a regression in which
>> devices that don't support VPD were being scanned for VPD attributes
>> anyway.  This could cause issues for this parts and should be avoided so
>> the check for scsi_level has been moved out of scsi_add_lun and into
>> scsi_attach_vpd so that all callers will not scan VPD for devices that
>> don't support it.
>>
>> Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
>> Signed-off-by: Alexander Duyck <aduyck@...antis.com>
>> ---
>>  drivers/scsi/scsi.c      |    3 +++
>>  drivers/scsi/scsi_scan.c |    3 +--
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index b1bf42b93fcc..ed085e78c893 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -784,6 +784,9 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>>       int pg83_supported = 0;
>>       unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
>>
>> +     if (sdev->scsi_level < SCSI_3)
>> +             return;
>> +
>>       if (sdev->skip_vpd_pages)
>>               return;
>>  retry_pg0:
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index 6a820668d442..1b16c89e0cf9 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -986,8 +986,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>>               }
>>       }
>>
>> -     if (sdev->scsi_level >= SCSI_3)
>> -             scsi_attach_vpd(sdev);
>> +     scsi_attach_vpd(sdev);
>>
>>       sdev->max_queue_depth = sdev->queue_depth;
>>
>>
> Isn't this slightly pointless, given that we're testing the inverse
> condition in scsi_attach_vpd()?

I'm not sure what you are getting at.  What I basically did is move
the check here into the function.  No point in checking it in 2 spots
when checking it inside the function is good enough.

> And in anycase, I guess we should be using the same logic sd.c is
> using. Please see the attached patch.

The attached patch looks good as it also takes care of the opt-in case
which I had overlooked.  The only bit missing is the fact that we are
still checking scsi_level twice when we don't need to.

- Alex