Hi Bart, Comments inline below about the show_vpd_##_page macro. > -----Original Message----- > From: Bart Van Assche [mailto:bart.vanassche@xxxxxxx] > Sent: Saturday, August 26, 2017 6:36 AM > To: Martin K . Petersen <martin.petersen@xxxxxxxxxx>; James E . J . > Bottomley <jejb@xxxxxxxxxxxxxxxxxx> > Cc: linux-scsi@xxxxxxxxxxxxxxx; Bart Van Assche <bart.vanassche@xxxxxxx>; > Christoph Hellwig <hch@xxxxxx>; Hannes Reinecke <hare@xxxxxxx>; > Johannes Thumshirn <jthumshirn@xxxxxxx>; Seymour, Shane M > <shane.seymour@xxxxxxx> > Subject: [PATCH 2/2] Rework handling of scsi_device.vpd_pg8[03] > > Introduce struct scsi_vpd for the VPD page length, data and the > RCU head that will be used to free the VPD data. Use kfree_rcu() > instead of kfree() to free VPD data. Only annotate pointers that > are shared across threads with __rcu. Use rcu_dereference() when > dereferencing an RCU pointer. This patch suppresses about twenty > sparse complaints about the vpd_pg8[03] pointers. > > Fixes: commit 09e2b0b14690 ("scsi: rescan VPD attributes") > Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxx> > Cc: Christoph Hellwig <hch@xxxxxx> > Cc: Hannes Reinecke <hare@xxxxxxx> > Cc: Johannes Thumshirn <jthumshirn@xxxxxxx> > Cc: Shane Seymour <shane.seymour@xxxxxxx> > --- > drivers/scsi/scsi.c | 48 +++++++++++++++++++++------------------------- > drivers/scsi/scsi_lib.c | 16 ++++++++-------- > drivers/scsi/scsi_sysfs.c | 26 +++++++++++++++++++------ > include/scsi/scsi_device.h | 18 +++++++++++++---- > 4 files changed, 64 insertions(+), 44 deletions(-) > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > index 775f609f89e2..1cf3aef0de8a 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -415,22 +415,20 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page); > * scsi_get_vpd_buf - Get Vital Product Data from a SCSI device > * @sdev: The device to ask > * @page: Which Vital Product Data to return > - * @len: Upon success, the VPD length will be stored in *@len. > * > * Returns %NULL upon failure. > */ > -static unsigned char *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page, > - int *len) > +static struct scsi_vpd *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page) > { > - unsigned char *vpd_buf; > + struct scsi_vpd *vpd_buf; > int vpd_len = SCSI_VPD_PG_LEN, result; > > retry_pg: > - vpd_buf = kmalloc(vpd_len, GFP_KERNEL); > + vpd_buf = kmalloc(sizeof(*vpd_buf) + vpd_len, GFP_KERNEL); > if (!vpd_buf) > return NULL; > > - result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len); > + result = scsi_vpd_inquiry(sdev, vpd_buf->data, page, vpd_len); > if (result < 0) { > kfree(vpd_buf); > return NULL; > @@ -441,7 +439,7 @@ static unsigned char *scsi_get_vpd_buf(struct > scsi_device *sdev, u8 page, > goto retry_pg; > } > > - *len = result; > + vpd_buf->len = result; > > return vpd_buf; > } > @@ -458,52 +456,50 @@ static unsigned char *scsi_get_vpd_buf(struct > scsi_device *sdev, u8 page, > void scsi_attach_vpd(struct scsi_device *sdev) > { > int i; > - int vpd_len; > int pg80_supported = 0; > int pg83_supported = 0; > - unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL; > + struct scsi_vpd *vpd_buf, *orig_vpd_buf; > > if (!scsi_device_supports_vpd(sdev)) > return; > > /* Ask for all the pages supported by this device */ > - vpd_buf = scsi_get_vpd_buf(sdev, 0, &vpd_len); > + vpd_buf = scsi_get_vpd_buf(sdev, 0); > if (!vpd_buf) > return; > > - for (i = 4; i < vpd_len; i++) { > - if (vpd_buf[i] == 0x80) > + for (i = 4; i < vpd_buf->len; i++) { > + if (vpd_buf->data[i] == 0x80) > pg80_supported = 1; > - if (vpd_buf[i] == 0x83) > + if (vpd_buf->data[i] == 0x83) > pg83_supported = 1; > } > kfree(vpd_buf); > > if (pg80_supported) { > - vpd_buf = scsi_get_vpd_buf(sdev, 0x80, &vpd_len); > + vpd_buf = scsi_get_vpd_buf(sdev, 0x80); > > mutex_lock(&sdev->inquiry_mutex); > - orig_vpd_buf = sdev->vpd_pg80; > - sdev->vpd_pg80_len = vpd_len; > + orig_vpd_buf = rcu_dereference_protected(sdev- > >vpd_pg80, > + lockdep_is_held(&sdev- > >inquiry_mutex)); > rcu_assign_pointer(sdev->vpd_pg80, vpd_buf); > mutex_unlock(&sdev->inquiry_mutex); > - synchronize_rcu(); > - if (orig_vpd_buf) { > - kfree(orig_vpd_buf); > - orig_vpd_buf = NULL; > - } > + > + if (orig_vpd_buf) > + kfree_rcu(orig_vpd_buf, rcu); > } > > if (pg83_supported) { > - vpd_buf = scsi_get_vpd_buf(sdev, 0x83, &vpd_len); > + vpd_buf = scsi_get_vpd_buf(sdev, 0x83); > + > mutex_lock(&sdev->inquiry_mutex); > - orig_vpd_buf = sdev->vpd_pg83; > - sdev->vpd_pg83_len = vpd_len; > + orig_vpd_buf = rcu_dereference_protected(sdev- > >vpd_pg83, > + lockdep_is_held(&sdev- > >inquiry_mutex)); > rcu_assign_pointer(sdev->vpd_pg83, vpd_buf); > mutex_unlock(&sdev->inquiry_mutex); > - synchronize_rcu(); > + > if (orig_vpd_buf) > - kfree(orig_vpd_buf); > + kfree_rcu(orig_vpd_buf, rcu); > } > } > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 813d2456ae93..3d6e50087600 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -3276,8 +3276,8 @@ 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; > + const unsigned char *d, *cur_id_str; > + const struct scsi_vpd *vpd_pg83; > int id_size = -EINVAL; > > rcu_read_lock(); > @@ -3308,8 +3308,8 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char > *id, size_t id_len) > } > > memset(id, 0, id_len); > - d = vpd_pg83 + 4; > - while (d < vpd_pg83 + sdev->vpd_pg83_len) { > + d = vpd_pg83->data + 4; > + while (d < vpd_pg83->data + vpd_pg83->len) { > /* Skip designators not referring to the LUN */ > if ((d[1] & 0x30) != 0x00) > goto next_desig; > @@ -3425,8 +3425,8 @@ EXPORT_SYMBOL(scsi_vpd_lun_id); > */ > int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id) > { > - unsigned char *d; > - unsigned char __rcu *vpd_pg83; > + const unsigned char *d; > + const struct scsi_vpd *vpd_pg83; > int group_id = -EAGAIN, rel_port = -1; > > rcu_read_lock(); > @@ -3436,8 +3436,8 @@ int scsi_vpd_tpg_id(struct scsi_device *sdev, int > *rel_id) > return -ENXIO; > } > > - d = sdev->vpd_pg83 + 4; > - while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) { > + d = vpd_pg83->data + 4; > + while (d < vpd_pg83->data + vpd_pg83->len) { > switch (d[1] & 0xf) { > case 0x4: > /* Relative target port */ > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c > index 5ed473a87589..8a75e0f10eeb 100644 > --- a/drivers/scsi/scsi_sysfs.c > +++ b/drivers/scsi/scsi_sysfs.c > @@ -428,6 +428,7 @@ static void > scsi_device_dev_release_usercontext(struct work_struct *work) > struct scsi_device *sdev; > struct device *parent; > struct list_head *this, *tmp; > + struct scsi_vpd *vpd_pg80, *vpd_pg83; > unsigned long flags; > > sdev = container_of(work, struct scsi_device, ew.work); > @@ -456,8 +457,19 @@ static void > scsi_device_dev_release_usercontext(struct work_struct *work) > /* NULL queue means the device can't be used */ > sdev->request_queue = NULL; > > - kfree(sdev->vpd_pg83); > - kfree(sdev->vpd_pg80); > + mutex_lock(&sdev->inquiry_mutex); > + vpd_pg80 = rcu_dereference_protected(sdev->vpd_pg80, > + lockdep_is_held(&sdev->inquiry_mutex)); > + rcu_assign_pointer(sdev->vpd_pg80, NULL); > + vpd_pg83 = rcu_dereference_protected(sdev->vpd_pg83, > + lockdep_is_held(&sdev->inquiry_mutex)); > + rcu_assign_pointer(sdev->vpd_pg83, NULL); > + mutex_unlock(&sdev->inquiry_mutex); > + > + if (vpd_pg83) > + kfree_rcu(vpd_pg83, rcu); > + if (vpd_pg80) > + kfree_rcu(vpd_pg80, rcu); > kfree(sdev->inquiry); > kfree(sdev); > > @@ -795,15 +807,17 @@ show_vpd_##_page(struct file *filp, struct kobject > *kobj, \ > { \ > struct device *dev = container_of(kobj, struct device, kobj); \ > struct scsi_device *sdev = to_scsi_device(dev); > \ > + struct scsi_vpd *vpd_page; \ > int ret; \ > + \ Can this check here: > if (!sdev->vpd_##_page) > \ > return -EINVAL; > \ > rcu_read_lock(); \ > - ret = memory_read_from_buffer(buf, count, &off, > \ > - rcu_dereference(sdev->vpd_##_page), \ > - sdev->vpd_##_page##_len); \ > + vpd_page = rcu_dereference(sdev->vpd_##_page); Be moved to here so memory_read_from_buffer is made conditional on if vpd_page is not NULL (initialize ret with -EINVAL)? Although it's been checked above there's no guarantee that vpd_page may now be set to. > \ > + ret = memory_read_from_buffer(buf, count, &off, vpd_page->data, > \ > + vpd_page->len); \ > rcu_read_unlock(); \ > - return ret; \ > + return ret; \ > } \ That should make it look more like this: struct device *dev = container_of(kobj, struct device, kobj); \ struct scsi_device *sdev = to_scsi_device(dev); \ + struct scsi_vpd *vpd_page; \ - int ret; \ - int ret = -EINVAL; \ + \ - if (!sdev->vpd_##_page) \ - return -EINVAL; \ rcu_read_lock(); \ - ret = memory_read_from_buffer(buf, count, &off, \ - rcu_dereference(sdev->vpd_##_page), \ - sdev->vpd_##_page##_len); \ + vpd_page = rcu_dereference(sdev->vpd_##_page); \ + if (vpd_page) \ + ret = memory_read_from_buffer(buf, count, &off, \ + vpd_page->data, vpd_page->len); \ rcu_read_unlock(); \ - return ret; \ + return ret; \ If you're happy to make a change like that feel free to add a reviewed-by line from me. > static struct bin_attribute dev_attr_vpd_##_page = { \ > .attr = {.name = __stringify(vpd_##_page), .mode = S_IRUGO }, > \ > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h > index f054f3f43c75..82e93ee94708 100644 > --- a/include/scsi/scsi_device.h > +++ b/include/scsi/scsi_device.h > @@ -80,6 +80,18 @@ struct scsi_event { > */ > }; > > +/** > + * struct scsi_vpd - SCSI Vital Product Data > + * @rcu: For kfree_rcu(). > + * @len: Length in bytes of @data. > + * @data: VPD data as defined in various T10 SCSI standard documents. > + */ > +struct scsi_vpd { > + struct rcu_head rcu; > + int len; > + unsigned char data[]; > +}; > + > struct scsi_device { > struct Scsi_Host *host; > struct request_queue *request_queue; > @@ -122,10 +134,8 @@ struct scsi_device { > const char * rev; /* ... "nullnullnullnull" before scan */ > > #define SCSI_VPD_PG_LEN 255 > - int vpd_pg83_len; > - unsigned char __rcu *vpd_pg83; > - int vpd_pg80_len; > - unsigned char __rcu *vpd_pg80; > + struct scsi_vpd __rcu *vpd_pg83; > + struct scsi_vpd __rcu *vpd_pg80; > unsigned char current_tag; /* current tag */ > struct scsi_target *sdev_target; /* used only for single_lun */ > > -- > 2.14.1
