[PATCH 2/2] scsi: Fix RCU handling for VPD pages

2016-01-20 Thread Alexander Duyck
This patch is meant to fix the RCU handling for VPD pages.  The original
code had a number of issues including the fact that the local variables
were being declared as __rcu, the RCU variable being directly accessed
outside of the RCU locked region, and the fact that length was not
associated with the data so it would be possible to get a mix and match of
the length for one VPD page with the data from another.

Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
Signed-off-by: Alexander Duyck 
---
 drivers/scsi/scsi.c|   52 +++-
 drivers/scsi/scsi_lib.c|   12 +-
 drivers/scsi/scsi_sysfs.c  |   14 +++-
 include/scsi/scsi_device.h |   14 
 4 files changed, 50 insertions(+), 42 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ed085e78c893..143b384fd145 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -782,7 +782,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
int vpd_len = SCSI_VPD_PG_LEN;
int pg80_supported = 0;
int pg83_supported = 0;
-   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+   unsigned char *vpd_buf;
 
if (sdev->scsi_level < SCSI_3)
return;
@@ -816,58 +816,60 @@ retry_pg0:
vpd_len = SCSI_VPD_PG_LEN;
 
if (pg80_supported) {
+   struct scsi_vpd_pg *vpd, *orig_vpd;
 retry_pg80:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
+   vpd = kmalloc(sizeof(*vpd) + vpd_len, GFP_KERNEL);
+   if (!vpd)
return;
 
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
+   result = scsi_vpd_inquiry(sdev, vpd->buf, 0x80, vpd_len);
if (result < 0) {
-   kfree(vpd_buf);
+   kfree(vpd);
return;
}
if (result > vpd_len) {
vpd_len = result;
-   kfree(vpd_buf);
+   kfree(vpd);
goto retry_pg80;
}
+   vpd->len = result;
+
mutex_lock(>inquiry_mutex);
-   orig_vpd_buf = sdev->vpd_pg80;
-   sdev->vpd_pg80_len = result;
-   rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
+   orig_vpd = rcu_dereference_protected(sdev->vpd_pg80, 1);
+   rcu_assign_pointer(sdev->vpd_pg80, vpd);
mutex_unlock(>inquiry_mutex);
-   synchronize_rcu();
-   if (orig_vpd_buf) {
-   kfree(orig_vpd_buf);
-   orig_vpd_buf = NULL;
-   }
+
+   if (orig_vpd)
+   kfree_rcu(orig_vpd, rcu);
vpd_len = SCSI_VPD_PG_LEN;
}
 
if (pg83_supported) {
+   struct scsi_vpd_pg *vpd, *orig_vpd;
 retry_pg83:
-   vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-   if (!vpd_buf)
+   vpd = kmalloc(sizeof(*vpd) + vpd_len, GFP_KERNEL);
+   if (!vpd)
return;
 
-   result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
+   result = scsi_vpd_inquiry(sdev, vpd->buf, 0x83, vpd_len);
if (result < 0) {
-   kfree(vpd_buf);
+   kfree(vpd);
return;
}
if (result > vpd_len) {
vpd_len = result;
-   kfree(vpd_buf);
+   kfree(vpd);
goto retry_pg83;
}
+   vpd->len = result;
+
mutex_lock(>inquiry_mutex);
-   orig_vpd_buf = sdev->vpd_pg83;
-   sdev->vpd_pg83_len = result;
-   rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
+   orig_vpd = rcu_dereference_protected(sdev->vpd_pg83, 1);
+   rcu_assign_pointer(sdev->vpd_pg83, vpd);
mutex_unlock(>inquiry_mutex);
-   synchronize_rcu();
-   if (orig_vpd_buf)
-   kfree(orig_vpd_buf);
+
+   if (orig_vpd)
+   kfree_rcu(orig_vpd, rcu);
}
 }
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fa6b2c4eb7a2..e44f66bc4c90 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3175,7 +3175,7 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, 
size_t id_len)
u8 cur_id_type = 0xff;
u8 cur_id_size = 0;
unsigned char *d, *cur_id_str;
-   unsigned char __rcu *vpd_pg83;
+   struct scsi_vpd_pg *vpd_pg83;
int id_size = -EINVAL;
 
rcu_read_lock();
@@ -3205,8 +3205,8 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, 
size_t id_len)
}
 
memset(id, 0, id_len);
-   d = 

Re: [PATCH 2/2] scsi: Fix RCU handling for VPD pages

2016-01-20 Thread Hannes Reinecke
On 01/21/2016 07:35 AM, Alexander Duyck wrote:
> This patch is meant to fix the RCU handling for VPD pages.  The original
> code had a number of issues including the fact that the local variables
> were being declared as __rcu, the RCU variable being directly accessed
> outside of the RCU locked region, and the fact that length was not
> associated with the data so it would be possible to get a mix and match of
> the length for one VPD page with the data from another.
> 
> Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
> Signed-off-by: Alexander Duyck 
> ---
>  drivers/scsi/scsi.c|   52 
> +++-
>  drivers/scsi/scsi_lib.c|   12 +-
>  drivers/scsi/scsi_sysfs.c  |   14 +++-
>  include/scsi/scsi_device.h |   14 
>  4 files changed, 50 insertions(+), 42 deletions(-)
> 
Thanks for fixing this up. I didn't really like the two distinct
variables for vpd buffer and length, too, but hadn't thought of
using a struct for here.

Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html