[linux-scsi] How to solve the delayed grant mapping problem with blkback?

[Xieyingtai]

Hello,
Recently I got a problem when I use virtual machines in Xen 
virtualization, description just as follws.
Grant table and grant mapping were used for data communication with in 
Xen front end driver and
back end driver. A delay work will be set up to release grant mapping 
pages in end_block_io_op as

bio callbacks, the code as below:
static void __gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item)
{
   int ret;
   int pc;

   for (pc = 0; pc < item->count; pc++) {
  if (page_count(item->pages[pc]) > 1) {
 unsigned long delay = GNTTAB_UNMAP_REFS_DELAY * 
(item->age + 1);

 schedule_delayed_work(>gnttab_work,
 msecs_to_jiffies(delay));
 return;
  }
   }

   ret = gnttab_unmap_refs(item->unmap_ops, item->kunmap_ops,
item->pages, item->count);
   item->done(ret, item);
}

Only if the page count of grant mapping page was decrease to one, a 
response would be made to
notify the vm front end. But in the scenario which a network cable was 
extracted, the TCP transport
layer will retransmit the socket data. As a result, the grant mapping 
pages were occupied by
the TCP retransmition which may continued for a long time. And then in 
blkback driver, the grant unmap
delay work will continue until the TCP releases all the pages. As an 
application of TCP protocol, Is there any
method to solve this problem in iscsi driver layer? I have already 
attempted to modified some parameters
provide by iscsi, but it was a futile effort. The socket connection 
seems not be closed in the network cable

extracted scenario.

Thanks

[PATCH 2/3] scsi_dh_alua: Ensure that alua_activate() calls the completion function

[Bart Van Assche]

Callers of scsi_dh_activate(), e.g. dm-mpath, assume that this
function either returns an error code or calls the completion
function. Make alua_activate() call the completion function even
if scsi_device_get() fails.

Signed-off-by: Bart Van Assche 
Cc: Hannes Reinecke 
Cc: Tang Junhui 
Cc: 
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
b/drivers/scsi/device_handler/scsi_dh_alua.c
index e0b15f3dd303..b6849d3ecefe 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -113,7 +113,7 @@ struct alua_queue_data {
 #define ALUA_POLICY_SWITCH_ALL 1
 
 static void alua_rtpg_work(struct work_struct *work);
-static void alua_rtpg_queue(struct alua_port_group *pg,
+static bool alua_rtpg_queue(struct alua_port_group *pg,
struct scsi_device *sdev,
struct alua_queue_data *qdata, bool force);
 static void alua_check(struct scsi_device *sdev, bool force);
@@ -862,7 +862,13 @@ static void alua_rtpg_work(struct work_struct *work)
kref_put(>kref, release_port_group);
 }
 
-static void alua_rtpg_queue(struct alua_port_group *pg,
+/**
+ * alua_rtpg_queue() - cause RTPG to be submitted asynchronously
+ *
+ * Returns true if and only if alua_rtpg_work() will be called asynchronously.
+ * That function is responsible for calling @qdata->fn().
+ */
+static bool alua_rtpg_queue(struct alua_port_group *pg,
struct scsi_device *sdev,
struct alua_queue_data *qdata, bool force)
 {
@@ -871,7 +877,7 @@ static void alua_rtpg_queue(struct alua_port_group *pg,
struct workqueue_struct *alua_wq = kaluad_wq;
 
if (!pg || scsi_device_get(sdev))
-   return;
+   return false;
 
spin_lock_irqsave(>lock, flags);
if (qdata) {
@@ -907,6 +913,8 @@ static void alua_rtpg_queue(struct alua_port_group *pg,
}
if (sdev)
scsi_device_put(sdev);
+
+   return true;
 }
 
 /*
@@ -1007,11 +1015,13 @@ static int alua_activate(struct scsi_device *sdev,
mutex_unlock(>init_mutex);
goto out;
}
-   fn = NULL;
rcu_read_unlock();
mutex_unlock(>init_mutex);
 
-   alua_rtpg_queue(pg, sdev, qdata, true);
+   if (alua_rtpg_queue(pg, sdev, qdata, true))
+   fn = NULL;
+   else
+   err = SCSI_DH_DEV_OFFLINED;
kref_put(>kref, release_port_group);
 out:
if (fn)
-- 
2.12.0

[PATCH 1/3] scsi_dh_alua: Check scsi_device_get() return value

[Bart Van Assche]

Do not queue ALUA work nor call scsi_device_put() if the scsi_device_get()
call fails. This patch fixes the following crash:

general protection fault:  [#1] SMP
RIP: 0010:scsi_device_put+0xb/0x30
Call Trace:
 scsi_disk_put+0x2d/0x40
 sd_release+0x3d/0xb0
 __blkdev_put+0x29e/0x360
 blkdev_put+0x49/0x170
 dm_put_table_device+0x58/0xc0 [dm_mod]
 dm_put_device+0x70/0xc0 [dm_mod]
 free_priority_group+0x92/0xc0 [dm_multipath]
 free_multipath+0x70/0xc0 [dm_multipath]
 multipath_dtr+0x19/0x20 [dm_multipath]
 dm_table_destroy+0x67/0x120 [dm_mod]
 dev_suspend+0xde/0x240 [dm_mod]
 ctl_ioctl+0x1f5/0x520 [dm_mod]
 dm_ctl_ioctl+0xe/0x20 [dm_mod]
 do_vfs_ioctl+0x8f/0x700
 SyS_ioctl+0x3c/0x70
 entry_SYSCALL_64_fastpath+0x18/0xad

Fixes: commit 03197b61c5ec ("scsi_dh_alua: Use workqueue for RTPG")
Signed-off-by: Bart Van Assche 
Cc: Hannes Reinecke 
Cc: Tang Junhui 
Cc: 
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
b/drivers/scsi/device_handler/scsi_dh_alua.c
index 48e200102221..e0b15f3dd303 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -870,7 +870,7 @@ static void alua_rtpg_queue(struct alua_port_group *pg,
unsigned long flags;
struct workqueue_struct *alua_wq = kaluad_wq;
 
-   if (!pg)
+   if (!pg || scsi_device_get(sdev))
return;
 
spin_lock_irqsave(>lock, flags);
@@ -884,14 +884,12 @@ static void alua_rtpg_queue(struct alua_port_group *pg,
pg->flags |= ALUA_PG_RUN_RTPG;
kref_get(>kref);
pg->rtpg_sdev = sdev;
-   scsi_device_get(sdev);
start_queue = 1;
} else if (!(pg->flags & ALUA_PG_RUN_RTPG) && force) {
pg->flags |= ALUA_PG_RUN_RTPG;
/* Do not queue if the worker is already running */
if (!(pg->flags & ALUA_PG_RUNNING)) {
kref_get(>kref);
-   sdev = NULL;
start_queue = 1;
}
}
@@ -900,13 +898,15 @@ static void alua_rtpg_queue(struct alua_port_group *pg,
alua_wq = kaluad_sync_wq;
spin_unlock_irqrestore(>lock, flags);
 
-   if (start_queue &&
-   !queue_delayed_work(alua_wq, >rtpg_work,
-   msecs_to_jiffies(ALUA_RTPG_DELAY_MSECS))) {
-   if (sdev)
-   scsi_device_put(sdev);
-   kref_put(>kref, release_port_group);
+   if (start_queue) {
+   if (queue_delayed_work(alua_wq, >rtpg_work,
+   msecs_to_jiffies(ALUA_RTPG_DELAY_MSECS)))
+   sdev = NULL;
+   else
+   kref_put(>kref, release_port_group);
}
+   if (sdev)
+   scsi_device_put(sdev);
 }
 
 /*
-- 
2.12.0

Re: [PATCH v2] scsi_sysfs: fix hang when removing scsi device

[Bart Van Assche]

On Thu, 2017-03-16 at 23:00 +, Bart Van Assche wrote:
> The following crash only occurs with async aborts enabled:
> 
> general protection fault:  [#1] SMP
> RIP: 0010:scsi_device_put+0xb/0x30
> Call Trace:
>  scsi_disk_put+0x2d/0x40
>  sd_release+0x3d/0xb0
>  __blkdev_put+0x29e/0x360
>  blkdev_put+0x49/0x170
>  dm_put_table_device+0x58/0xc0 [dm_mod]
>  dm_put_device+0x70/0xc0 [dm_mod]
>  free_priority_group+0x92/0xc0 [dm_multipath]
>  free_multipath+0x70/0xc0 [dm_multipath]
>  multipath_dtr+0x19/0x20 [dm_multipath]
>  dm_table_destroy+0x67/0x120 [dm_mod]
>  dev_suspend+0xde/0x240 [dm_mod]
>  ctl_ioctl+0x1f5/0x520 [dm_mod]
>  dm_ctl_ioctl+0xe/0x20 [dm_mod]
>  do_vfs_ioctl+0x8f/0x700
>  SyS_ioctl+0x3c/0x70
>  entry_SYSCALL_64_fastpath+0x18/0xad

(replying to my own e-mail)

With my three scsi_dh_alua patches applied I don't see this crash anymore so
this crash was probably unrelated to async aborts.

Bart.

[PATCH 0/3] SCSI ALUA device handler bug fixes

[Bart Van Assche]

Hello Martin,

These three patches are what I came up with while I was chasing a
scsi_device_put() crash. Please consider these for inclusion in the
upstream kernel.

Thanks,

Bart.

Bart Van Assche (3):
  scsi_dh_alua: Check scsi_device_get() return value
  scsi_dh_alua: Ensure that alua_activate() calls the completion
function
  scsi_dh_alua: Warn if the first argument of alua_rtpg_queue() is NULL

 drivers/scsi/device_handler/scsi_dh_alua.c | 38 +++---
 1 file changed, 24 insertions(+), 14 deletions(-)

-- 
2.12.0

[PATCH 3/3] scsi_dh_alua: Warn if the first argument of alua_rtpg_queue() is NULL

[Bart Van Assche]

Callers must provide a valid port group to alua_rtpg_queue().
Issue a kernel warning if that is not the case.

Signed-off-by: Bart Van Assche 
Cc: Hannes Reinecke 
Cc: Tang Junhui 
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
b/drivers/scsi/device_handler/scsi_dh_alua.c
index b6849d3ecefe..c01b47e5b55a 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -876,7 +876,7 @@ static bool alua_rtpg_queue(struct alua_port_group *pg,
unsigned long flags;
struct workqueue_struct *alua_wq = kaluad_wq;
 
-   if (!pg || scsi_device_get(sdev))
+   if (WARN_ON_ONCE(!pg) || scsi_device_get(sdev))
return false;
 
spin_lock_irqsave(>lock, flags);
-- 
2.12.0

Re: [PATCH] scsi: sr: fix oob access in get_capabilities

[Martin K. Petersen]

Kefeng Wang  writes:

Kefeng,

> root@localhost ~]# sg_modes -p 0x2a /dev/sr0
> QEMU  QEMU DVD-ROM  0.15   peripheral_type: cd/dvd [0x5]
> Mode parameter header from MODE SENSE(10):
> Invalid block descriptor length=512, ignore
>   Mode data length=36, medium type=0x70, specific param=0x00, longlba=0
>   Block descriptor length=0
>>> MM capabilities and mechanical status (obsolete), page_control: current
>  00 2a 12 00 00 71 60 29 00  02 c2 00 02 02 00 02 c2
>  10 00 00 00 00
> Unexpectedly received extra mode page responses, ignore

That looks pretty broken.

Could you try the following patch?

Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f41e6b84a1bd..51a4ce094450 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2485,6 +2485,10 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int 
modepage,
goto retry;
}
 
+   if (data->length > len ||
+   data->header_length + data->block_descriptor_length > data->length)
+   return 0;
+
return result;
 }
 EXPORT_SYMBOL(scsi_mode_sense);

Re: [RFC 1/2] scsi: Provide mechanism for SCSI layer to poll for LLDD.

[Bart Van Assche]

On Fri, 2017-03-17 at 18:55 +, Madhani, Himanshu wrote:
> On Mar 16, 2017, at 3:27 PM, Bart Van Assche  
> wrote:
> > On Thu, 2017-03-16 at 14:40 -0700, Himanshu Madhani wrote:
> > > +static int
> > > +scsi_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
> > > +
> > > +{
> > > +struct Scsi_Host *shost = hctx->driver_data;
> > > +struct request *req;
> > > +struct scsi_cmnd *cmd;
> > > +
> > > +req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], 
> > > tag);
> > > +if (!req)
> > > +return 0;
> > > +
> > > +cmd = blk_mq_rq_to_pdu(req);
> > > + if (!cmd)
> > > + return 0;
> > > +
> > > + if (shost->hostt->poll_queue)
> > > + return(shost->hostt->poll_queue(shost, cmd));
> > > + else return 0;
> > > +}
> > 
> > What will happen if the request with tag 'tag' is completed after the block
> > layer decided to call this function and before this function had the chance
> > to call blk_mq_tag_to_rq()? Will this trigger a kernel crash? Also, please
> > format patches properly. This is what checkpatch reports for this patch:
> > 
> 
> Tag is passed into here by rq->tag
> 
> When tag goes to blk_mq_tag_to_rq  it just indexes to the rqs array using tag:
> 
> struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag)
> {
>if (tag < tags->nr_tags) {
>prefetch(tags->rqs[tag]);
>return tags->rqs[tag];
>}
> 
>return NULL;
> 
> So if tag is invalid(too large), it returns NULL.  Every step it validates it 
> has a valid * before continuing.  
> 
> Worse case we poll for a completion that has already completed. 
> 
> We don’t think this will result into NULL pointer crash.

Hello Himanshu,

Have you noticed that the caller of scsi_poll(), namely blk_mq_poll(), does
not use any kind of mechanism to prevent that the 'tag' argument is freed and
reused while blk_mq_poll() is in progress? I think that a SCSI completion
can occur while blk_mq_poll() is in progress. What will happen if a SCSI
completion occurs concurrently with scsi_poll()? Will that trigger a
use-after-free of the scsi_cmnd structure 'cmd' points at? If so, what will
be the consequences?

Because of this I'm not sure we should proceed with adding blk_mq_poll()
support to the SCSI core. But if such support is added it's probably a much
better choice to change the type of the second argument of .poll_queue()
from struct scsi_cmnd * into int (the tag number). It is then the
responsibility of SCSI LLD's to avoid that their .poll_queue() implementation
triggers a use-after-free.

Bart.

[Bug 194837] VM with virtio-scsi drive often crashes during boot with kernel 4.11rc1

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=194837

--- Comment #15 from Thorsten Leemhuis (regressi...@leemhuis.info) ---
Adam or somebody else: Can you please close this Bug to avoid confusion? There
clearly were some bugs in virtio_scsi, but those were already fixed. 

But there are more bugs (at least for me): I bisectected the problem I'm still
seeing with todays mainline and found that "virtio_pci: use shared interrupts
for virtqueues" (5c34d002dcc7; from hch and mst) was the first bad commit for
me. Filed Bug 194911 to track that.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[PATCH v5 10/16] iio:core: utilize new cdev_device_add helper function

[Logan Gunthorpe]

Replace the open coded registration of the cdev and dev with the
new device_add_cdev() helper. The helper replaces a common pattern by
taking the proper reference against the parent device and adding both
the cdev and the device.

In doing so we have to remove a guard statement from cdev_del,
but this doesn't appear to be required in any way.

Signed-off-by: Logan Gunthorpe 
---
 drivers/iio/industrialio-core.c | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index d18ded4..26a03c6 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1720,18 +1720,13 @@ int iio_device_register(struct iio_dev *indio_dev)
 
cdev_init(_dev->chrdev, _buffer_fileops);
indio_dev->chrdev.owner = indio_dev->info->driver_module;
-   indio_dev->chrdev.kobj.parent = _dev->dev.kobj;
-   ret = cdev_add(_dev->chrdev, indio_dev->dev.devt, 1);
-   if (ret < 0)
-   goto error_unreg_eventset;
 
-   ret = device_add(_dev->dev);
+   ret = cdev_device_add(_dev->chrdev, _dev->dev);
if (ret < 0)
-   goto error_cdev_del;
+   goto error_unreg_eventset;
 
return 0;
-error_cdev_del:
-   cdev_del(_dev->chrdev);
+
 error_unreg_eventset:
iio_device_unregister_eventset(indio_dev);
 error_free_sysfs:
@@ -1752,10 +1747,8 @@ void iio_device_unregister(struct iio_dev *indio_dev)
 {
mutex_lock(_dev->info_exist_lock);
 
-   device_del(_dev->dev);
+   cdev_device_del(_dev->chrdev, _dev->dev);
 
-   if (indio_dev->chrdev.dev)
-   cdev_del(_dev->chrdev);
iio_device_unregister_debugfs(indio_dev);
 
iio_disable_all_buffers(indio_dev);
-- 
2.1.4

[PATCH v5 11/16] media: utilize new cdev_device_add helper function

[Logan Gunthorpe]

Replace the open coded registration of the cdev and dev with the
new device_add_cdev() helper. The helper replaces a common pattern by
taking the proper reference against the parent device and adding both
the cdev and the device.

Signed-off-by: Logan Gunthorpe 
Acked-by: Hans Verkuil 
---
 drivers/media/cec/cec-core.c  | 16 
 drivers/media/media-devnode.c | 20 +---
 2 files changed, 9 insertions(+), 27 deletions(-)

diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index 37217e2..3163e03 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -137,24 +137,17 @@ static int __must_check cec_devnode_register(struct 
cec_devnode *devnode,
 
/* Part 2: Initialize and register the character device */
cdev_init(>cdev, _devnode_fops);
-   devnode->cdev.kobj.parent = >dev.kobj;
devnode->cdev.owner = owner;
 
-   ret = cdev_add(>cdev, devnode->dev.devt, 1);
-   if (ret < 0) {
-   pr_err("%s: cdev_add failed\n", __func__);
+   ret = cdev_device_add(>cdev, >dev);
+   if (ret) {
+   pr_err("%s: cdev_device_add failed\n", __func__);
goto clr_bit;
}
 
-   ret = device_add(>dev);
-   if (ret)
-   goto cdev_del;
-
devnode->registered = true;
return 0;
 
-cdev_del:
-   cdev_del(>cdev);
 clr_bit:
mutex_lock(_devnode_lock);
clear_bit(devnode->minor, cec_devnode_nums);
@@ -190,8 +183,7 @@ static void cec_devnode_unregister(struct cec_devnode 
*devnode)
devnode->unregistered = true;
mutex_unlock(>lock);
 
-   device_del(>dev);
-   cdev_del(>cdev);
+   cdev_device_del(>cdev, >dev);
put_device(>dev);
 }
 
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index ae46753..423248f 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -248,31 +248,22 @@ int __must_check media_devnode_register(struct 
media_device *mdev,
dev_set_name(>dev, "media%d", devnode->minor);
device_initialize(>dev);
 
-   /* Part 2: Initialize and register the character device */
+   /* Part 2: Initialize the character device */
cdev_init(>cdev, _devnode_fops);
devnode->cdev.owner = owner;
-   devnode->cdev.kobj.parent = >dev.kobj;
 
-   ret = cdev_add(>cdev, MKDEV(MAJOR(media_dev_t), 
devnode->minor), 1);
+   /* Part 3: Add the media and char device */
+   ret = cdev_device_add(>cdev, >dev);
if (ret < 0) {
-   pr_err("%s: cdev_add failed\n", __func__);
+   pr_err("%s: cdev_device_add failed\n", __func__);
goto cdev_add_error;
}
 
-   /* Part 3: Add the media device */
-   ret = device_add(>dev);
-   if (ret < 0) {
-   pr_err("%s: device_add failed\n", __func__);
-   goto device_add_error;
-   }
-
/* Part 4: Activate this minor. The char device can now be used. */
set_bit(MEDIA_FLAG_REGISTERED, >flags);
 
return 0;
 
-device_add_error:
-   cdev_del(>cdev);
 cdev_add_error:
mutex_lock(_devnode_lock);
clear_bit(devnode->minor, media_devnode_nums);
@@ -298,9 +289,8 @@ void media_devnode_unregister(struct media_devnode *devnode)
 {
mutex_lock(_devnode_lock);
/* Delete the cdev on this minor as well */
-   cdev_del(>cdev);
+   cdev_device_del(>cdev, >dev);
mutex_unlock(_devnode_lock);
-   device_del(>dev);
devnode->media_dev = NULL;
put_device(>dev);
 }
-- 
2.1.4

[PATCH v5 07/16] platform/chrome: cros_ec_dev - utilize new cdev_device_add helper function

[Logan Gunthorpe]

Replace the open coded registration of the cdev and dev with the
new device_add_cdev() helper. The helper replaces a common pattern by
taking the proper reference against the parent device and adding both
the cdev and the device.

At the same time we cleanup the error path through device_probe
function: we use put_device instead of kfree directly as recommended
by the device_initialize documentation.

Signed-off-by: Logan Gunthorpe 
---
 drivers/platform/chrome/cros_ec_dev.c | 31 +++
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_dev.c 
b/drivers/platform/chrome/cros_ec_dev.c
index 6f09da4..6aa120c 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -391,7 +391,6 @@ static int ec_device_probe(struct platform_device *pdev)
int retval = -ENOMEM;
struct device *dev = >dev;
struct cros_ec_platform *ec_platform = dev_get_platdata(dev);
-   dev_t devno = MKDEV(ec_major, pdev->id);
struct cros_ec_dev *ec = kzalloc(sizeof(*ec), GFP_KERNEL);
 
if (!ec)
@@ -407,23 +406,11 @@ static int ec_device_probe(struct platform_device *pdev)
cdev_init(>cdev, );
 
/*
-* Add the character device
-* Link cdev to the class device to be sure device is not used
-* before unbinding it.
-*/
-   ec->cdev.kobj.parent = >class_dev.kobj;
-   retval = cdev_add(>cdev, devno, 1);
-   if (retval) {
-   dev_err(dev, ": failed to add character device\n");
-   goto cdev_add_failed;
-   }
-
-   /*
 * Add the class device
 * Link to the character device for creating the /dev entry
 * in devtmpfs.
 */
-   ec->class_dev.devt = ec->cdev.dev;
+   ec->class_dev.devt = MKDEV(ec_major, pdev->id);
ec->class_dev.class = _class;
ec->class_dev.parent = dev;
ec->class_dev.release = __remove;
@@ -431,13 +418,13 @@ static int ec_device_probe(struct platform_device *pdev)
retval = dev_set_name(>class_dev, "%s", ec_platform->ec_name);
if (retval) {
dev_err(dev, "dev_set_name failed => %d\n", retval);
-   goto set_named_failed;
+   goto failed;
}
 
-   retval = device_add(>class_dev);
+   retval = cdev_device_add(>cdev, >class_dev);
if (retval) {
-   dev_err(dev, "device_register failed => %d\n", retval);
-   goto dev_reg_failed;
+   dev_err(dev, "cdev_device_add failed => %d\n", retval);
+   goto failed;
}
 
/* check whether this EC is a sensor hub. */
@@ -446,12 +433,8 @@ static int ec_device_probe(struct platform_device *pdev)
 
return 0;
 
-dev_reg_failed:
-set_named_failed:
-   dev_set_drvdata(dev, NULL);
-   cdev_del(>cdev);
-cdev_add_failed:
-   kfree(ec);
+failed:
+   put_device(>class_dev);
return retval;
 }
 
-- 
2.1.4

Re: [RFC 1/2] scsi: Provide mechanism for SCSI layer to poll for LLDD.

[Madhani, Himanshu]

Hi Bart, 

> On Mar 16, 2017, at 3:27 PM, Bart Van Assche  
> wrote:
> 
> On Thu, 2017-03-16 at 14:40 -0700, Himanshu Madhani wrote:
>> +static int
>> +scsi_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
>> +
>> +{
>> +struct Scsi_Host *shost = hctx->driver_data;
>> +struct request *req;
>> +struct scsi_cmnd *cmd;
>> +
>> +req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);
>> +if (!req)
>> +return 0;
>> +
>> +cmd = blk_mq_rq_to_pdu(req);
>> +if (!cmd)
>> +return 0;
>> +
>> +if (shost->hostt->poll_queue)
>> +return(shost->hostt->poll_queue(shost, cmd));
>> +else return 0;
>> +}
> 
> What will happen if the request with tag 'tag' is completed after the block
> layer decided to call this function and before this function had the chance
> to call blk_mq_tag_to_rq()? Will this trigger a kernel crash? Also, please
> format patches properly. This is what checkpatch reports for this patch:
> 

Tag is passed into here by rq->tag

When tag goes to blk_mq_tag_to_rq  it just indexes to the rqs array using tag:

struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag)
{
   if (tag < tags->nr_tags) {
   prefetch(tags->rqs[tag]);
   return tags->rqs[tag];
   }

   return NULL;

So if tag is invalid(too large), it returns NULL.  Every step it validates it 
has a valid * before continuing.  

Worse case we poll for a completion that has already completed. 

We don’t think this will result into NULL pointer crash.

> ERROR: code indent should use tabs where possible
> #244: FILE: drivers/scsi/scsi_lib.c:2161:
> +struct Scsi_Host *shost = hctx->driver_data;$
> 
> WARNING: please, no spaces at the start of a line
> #244: FILE: drivers/scsi/scsi_lib.c:2161:
> +struct Scsi_Host *shost = hctx->driver_data;$
> 
> ERROR: code indent should use tabs where possible
> #245: FILE: drivers/scsi/scsi_lib.c:2162:
> +struct request *req;$
> 
> WARNING: please, no spaces at the start of a line
> #245: FILE: drivers/scsi/scsi_lib.c:2162:
> +struct request *req;$
> 
> ERROR: code indent should use tabs where possible
> #246: FILE: drivers/scsi/scsi_lib.c:2163:
> +struct scsi_cmnd *cmd;$
> 
> WARNING: please, no spaces at the start of a line
> #246: FILE: drivers/scsi/scsi_lib.c:2163:
> +struct scsi_cmnd *cmd;$
> 
> ERROR: code indent should use tabs where possible
> #248: FILE: drivers/scsi/scsi_lib.c:2165:
> +req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$
> 
> WARNING: please, no spaces at the start of a line
> #248: FILE: drivers/scsi/scsi_lib.c:2165:
> +req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$
> 
> ERROR: code indent should use tabs where possible
> #249: FILE: drivers/scsi/scsi_lib.c:2166:
> +if (!req)$
> 
> WARNING: please, no spaces at the start of a line
> #249: FILE: drivers/scsi/scsi_lib.c:2166:
> +if (!req)$
> 
> ERROR: code indent should use tabs where possible
> #250: FILE: drivers/scsi/scsi_lib.c:2167:
> +return 0;$
> 
> WARNING: please, no spaces at the start of a line
> #250: FILE: drivers/scsi/scsi_lib.c:2167:
> +return 0;$
> 
> ERROR: code indent should use tabs where possible
> #252: FILE: drivers/scsi/scsi_lib.c:2169:
> +cmd = blk_mq_rq_to_pdu(req);$
> 
> WARNING: please, no spaces at the start of a line
> #252: FILE: drivers/scsi/scsi_lib.c:2169:
> +cmd = blk_mq_rq_to_pdu(req);$
> 
> ERROR: return is not a function, parentheses are not required
> #257: FILE: drivers/scsi/scsi_lib.c:2174:
> + return(shost->hostt->poll_queue(shost, cmd));
> 
> ERROR: trailing statements should be on next line
> #258: FILE: drivers/scsi/scsi_lib.c:2175:
> + else return 0;
> 
> WARNING: line over 80 characters
> #262: FILE: drivers/scsi/scsi_lib.c:2179:
> +__scsi_init_hctx(struct blk_mq_hw_ctx *hctx, struct Scsi_Host *shost, 
> unsigned int hctx_idx)
> 
> WARNING: Unnecessary space before function pointer name
> #306: FILE: include/scsi/scsi_host.h:139:
> + int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *);
> 
> ERROR: space prohibited after that '*' (ctx:BxW)
> #306: FILE: include/scsi/scsi_host.h:139:
> + int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *);
>^
> Thanks,
> 
> Bart.

Thanks,
- Himanshu

[PATCH v5 12/16] mtd: utilize new cdev_device_add helper function

[Logan Gunthorpe]

This is not as straightforward a conversion as the others
in this series. These drivers did not originally make use of
kobj.parent so they likely suffered from a use after free bug if
someone unregistered the devices while they are being used.

In order to make the conversions, switch from device_register
to device_initialize / cdev_device_add.

In build.c, this patch unwinds a complicated mess of extra
get_device/put_devices and reference tracking by moving device_initialize
early in the attach process. Then it always uses put_device and instead of
using device_unregister and extra get_devices everywhere we just use
cdev_device_del and one put_device once everything is completely done.
This simplifies things dramatically and makes it easier to reason about.

In vmt.c, the patch pushes device initialization up to the beginning of the
device creation and then that function only needs to use put_device
in the error path which simplifies things a good deal.

Signed-off-by: Logan Gunthorpe 
---
 drivers/mtd/ubi/build.c | 91 +
 drivers/mtd/ubi/vmt.c   | 49 +-
 2 files changed, 33 insertions(+), 107 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 7751319..8bae373 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -421,41 +421,6 @@ static void dev_release(struct device *dev)
 }
 
 /**
- * ubi_sysfs_init - initialize sysfs for an UBI device.
- * @ubi: UBI device description object
- * @ref: set to %1 on exit in case of failure if a reference to @ubi->dev was
- *   taken
- *
- * This function returns zero in case of success and a negative error code in
- * case of failure.
- */
-static int ubi_sysfs_init(struct ubi_device *ubi, int *ref)
-{
-   int err;
-
-   ubi->dev.release = dev_release;
-   ubi->dev.devt = ubi->cdev.dev;
-   ubi->dev.class = _class;
-   ubi->dev.groups = ubi_dev_groups;
-   dev_set_name(>dev, UBI_NAME_STR"%d", ubi->ubi_num);
-   err = device_register(>dev);
-   if (err)
-   return err;
-
-   *ref = 1;
-   return 0;
-}
-
-/**
- * ubi_sysfs_close - close sysfs for an UBI device.
- * @ubi: UBI device description object
- */
-static void ubi_sysfs_close(struct ubi_device *ubi)
-{
-   device_unregister(>dev);
-}
-
-/**
  * kill_volumes - destroy all user volumes.
  * @ubi: UBI device description object
  */
@@ -471,27 +436,19 @@ static void kill_volumes(struct ubi_device *ubi)
 /**
  * uif_init - initialize user interfaces for an UBI device.
  * @ubi: UBI device description object
- * @ref: set to %1 on exit in case of failure if a reference to @ubi->dev was
- *   taken, otherwise set to %0
  *
  * This function initializes various user interfaces for an UBI device. If the
  * initialization fails at an early stage, this function frees all the
- * resources it allocated, returns an error, and @ref is set to %0. However,
- * if the initialization fails after the UBI device was registered in the
- * driver core subsystem, this function takes a reference to @ubi->dev, because
- * otherwise the release function ('dev_release()') would free whole @ubi
- * object. The @ref argument is set to %1 in this case. The caller has to put
- * this reference.
+ * resources it allocated, returns an error.
  *
  * This function returns zero in case of success and a negative error code in
  * case of failure.
  */
-static int uif_init(struct ubi_device *ubi, int *ref)
+static int uif_init(struct ubi_device *ubi)
 {
int i, err;
dev_t dev;
 
-   *ref = 0;
sprintf(ubi->ubi_name, UBI_NAME_STR "%d", ubi->ubi_num);
 
/*
@@ -508,20 +465,17 @@ static int uif_init(struct ubi_device *ubi, int *ref)
return err;
}
 
+   ubi->dev.devt = dev;
+
ubi_assert(MINOR(dev) == 0);
cdev_init(>cdev, _cdev_operations);
dbg_gen("%s major is %u", ubi->ubi_name, MAJOR(dev));
ubi->cdev.owner = THIS_MODULE;
 
-   err = cdev_add(>cdev, dev, 1);
-   if (err) {
-   ubi_err(ubi, "cannot add character device");
-   goto out_unreg;
-   }
-
-   err = ubi_sysfs_init(ubi, ref);
+   dev_set_name(>dev, UBI_NAME_STR "%d", ubi->ubi_num);
+   err = cdev_device_add(>cdev, >dev);
if (err)
-   goto out_sysfs;
+   goto out_unreg;
 
for (i = 0; i < ubi->vtbl_slots; i++)
if (ubi->volumes[i]) {
@@ -536,11 +490,7 @@ static int uif_init(struct ubi_device *ubi, int *ref)
 
 out_volumes:
kill_volumes(ubi);
-out_sysfs:
-   if (*ref)
-   get_device(>dev);
-   ubi_sysfs_close(ubi);
-   cdev_del(>cdev);
+   cdev_device_del(>cdev, >dev);
 out_unreg:
unregister_chrdev_region(ubi->cdev.dev, ubi->vtbl_slots + 1);
ubi_err(ubi, "cannot initialize UBI %s, error %d",
@@ -559,8 +509,7 @@ static int uif_init(struct ubi_device 

[PATCH v5 13/16] rapidio: utilize new cdev_device_add helper function

[Logan Gunthorpe]

This driver did not originally set kobj.parent so it likely had
potential a use after free bug which this patch fixes.

We convert from device_register to device_initialize/cdev_device_add.
While we are at it we use put_device instead of kfree (as recommended
by the device_initialize documentation). We also remove an unnecessary
extra get_device from the code.

Signed-off-by: Logan Gunthorpe 
---
 drivers/rapidio/devices/rio_mport_cdev.c | 24 
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/rapidio/devices/rio_mport_cdev.c 
b/drivers/rapidio/devices/rio_mport_cdev.c
index 50b617a..5beb0c3 100644
--- a/drivers/rapidio/devices/rio_mport_cdev.c
+++ b/drivers/rapidio/devices/rio_mport_cdev.c
@@ -2444,31 +2444,25 @@ static struct mport_dev *mport_cdev_add(struct 
rio_mport *mport)
mutex_init(>buf_mutex);
mutex_init(>file_mutex);
INIT_LIST_HEAD(>file_list);
-   cdev_init(>cdev, _fops);
-   md->cdev.owner = THIS_MODULE;
-   ret = cdev_add(>cdev, MKDEV(MAJOR(dev_number), mport->id), 1);
-   if (ret < 0) {
-   kfree(md);
-   rmcd_error("Unable to register a device, err=%d", ret);
-   return NULL;
-   }
 
-   md->dev.devt = md->cdev.dev;
+   device_initialize(>dev);
+   md->dev.devt = MKDEV(MAJOR(dev_number), mport->id);
md->dev.class = dev_class;
md->dev.parent = >dev;
md->dev.release = mport_device_release;
dev_set_name(>dev, DEV_NAME "%d", mport->id);
atomic_set(>active, 1);
 
-   ret = device_register(>dev);
+   cdev_init(>cdev, _fops);
+   md->cdev.owner = THIS_MODULE;
+
+   ret = cdev_device_add(>cdev, >dev);
if (ret) {
rmcd_error("Failed to register mport %d (err=%d)",
   mport->id, ret);
goto err_cdev;
}
 
-   get_device(>dev);
-
INIT_LIST_HEAD(>doorbells);
spin_lock_init(>db_lock);
INIT_LIST_HEAD(>portwrites);
@@ -2513,8 +2507,7 @@ static struct mport_dev *mport_cdev_add(struct rio_mport 
*mport)
return md;
 
 err_cdev:
-   cdev_del(>cdev);
-   kfree(md);
+   put_device(>dev);
return NULL;
 }
 
@@ -2578,7 +2571,7 @@ static void mport_cdev_remove(struct mport_dev *md)
atomic_set(>active, 0);
mport_cdev_terminate_dma(md);
rio_del_mport_pw_handler(md->mport, md, rio_mport_pw_handler);
-   cdev_del(&(md->cdev));
+   cdev_device_del(>cdev, >dev);
mport_cdev_kill_fasync(md);
 
flush_workqueue(dma_wq);
@@ -2603,7 +2596,6 @@ static void mport_cdev_remove(struct mport_dev *md)
 
rio_release_inb_dbell(md->mport, 0, 0x0fff);
 
-   device_unregister(>dev);
put_device(>dev);
 }
 
-- 
2.1.4

[PATCH v5 05/16] gpiolib: utilize new cdev_device_add helper function

[Logan Gunthorpe]

Replace the open coded registration of the cdev and dev with the
new device_add_cdev() helper. The helper replaces a common pattern by
taking the proper reference against the parent device and adding both
the cdev and the device.

Signed-off-by: Logan Gunthorpe 
Reviewed-by: Linus Walleij 
---
 drivers/gpio/gpiolib.c | 23 ---
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8b4d721..3ce2a27 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1035,18 +1035,14 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
 
cdev_init(>chrdev, _fileops);
gdev->chrdev.owner = THIS_MODULE;
-   gdev->chrdev.kobj.parent = >dev.kobj;
gdev->dev.devt = MKDEV(MAJOR(gpio_devt), gdev->id);
-   status = cdev_add(>chrdev, gdev->dev.devt, 1);
-   if (status < 0)
-   chip_warn(gdev->chip, "failed to add char device %d:%d\n",
- MAJOR(gpio_devt), gdev->id);
-   else
-   chip_dbg(gdev->chip, "added GPIO chardev (%d:%d)\n",
-MAJOR(gpio_devt), gdev->id);
-   status = device_add(>dev);
+
+   status = cdev_device_add(>chrdev, >dev);
if (status)
-   goto err_remove_chardev;
+   return status;
+
+   chip_dbg(gdev->chip, "added GPIO chardev (%d:%d)\n",
+MAJOR(gpio_devt), gdev->id);
 
status = gpiochip_sysfs_register(gdev);
if (status)
@@ -1061,9 +1057,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
return 0;
 
 err_remove_device:
-   device_del(>dev);
-err_remove_chardev:
-   cdev_del(>chrdev);
+   cdev_device_del(>chrdev, >dev);
return status;
 }
 
@@ -1347,8 +1341,7 @@ void gpiochip_remove(struct gpio_chip *chip)
 * be removed, else it will be dangling until the last user is
 * gone.
 */
-   cdev_del(>chrdev);
-   device_del(>dev);
+   cdev_device_del(>chrdev, >dev);
put_device(>dev);
 }
 EXPORT_SYMBOL_GPL(gpiochip_remove);
-- 
2.1.4

[PATCH v5 15/16] scsi: utilize new cdev_device_add helper function

[Logan Gunthorpe]

This driver did not set kobj.parent so it likely suffered from
a potential use after free race if the user unregistered the
device while it was in use.

This was not so straightforward a conversion but I think this patch
cleans up its probe's error path significantly.

This patch adds device_initialize, which is required for
cdev_device_add. Then it switches to put_device instead of kfree as
recommended by device_initialize's documentation. This removes a lot
from the error path which was already in __remove.
A couple things needed to be re-ordered to be entirely correct, though.
ida_remove is also moved out of __remove and into unregister to
simplify things and follow the pattern other devices are using.

This also drop an extra unnecessary get_device/put_device in the code.

Signed-off-by: Logan Gunthorpe 
---
 drivers/scsi/osd/osd_uld.c | 56 +-
 1 file changed, 20 insertions(+), 36 deletions(-)

diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
index e0ce5d2..4101c31 100644
--- a/drivers/scsi/osd/osd_uld.c
+++ b/drivers/scsi/osd/osd_uld.c
@@ -400,9 +400,6 @@ static void __remove(struct device *dev)
 
kfree(oud->odi.osdname);
 
-   if (oud->cdev.owner)
-   cdev_del(>cdev);
-
osd_dev_fini(>od);
scsi_device_put(scsi_device);
 
@@ -411,7 +408,6 @@ static void __remove(struct device *dev)
 
if (oud->disk)
put_disk(oud->disk);
-   ida_remove(_minor_ida, oud->minor);
 
kfree(oud);
 }
@@ -446,8 +442,20 @@ static int osd_probe(struct device *dev)
if (NULL == oud)
goto err_retract_minor;
 
+   /* class device member */
+   device_initialize(>class_dev);
dev_set_drvdata(dev, oud);
oud->minor = minor;
+   oud->class_dev.devt = MKDEV(SCSI_OSD_MAJOR, oud->minor);
+   oud->class_dev.class = _uld_class;
+   oud->class_dev.parent = dev;
+   oud->class_dev.release = __remove;
+
+   /* hold one more reference to the scsi_device that will get released
+* in __release, in case a logout is happening while fs is mounted
+*/
+   scsi_device_get(scsi_device);
+   osd_dev_init(>od, scsi_device);
 
/* allocate a disk and set it up */
/* FIXME: do we need this since sg has already done that */
@@ -461,59 +469,34 @@ static int osd_probe(struct device *dev)
sprintf(disk->disk_name, "osd%d", oud->minor);
oud->disk = disk;
 
-   /* hold one more reference to the scsi_device that will get released
-* in __release, in case a logout is happening while fs is mounted
-*/
-   scsi_device_get(scsi_device);
-   osd_dev_init(>od, scsi_device);
-
/* Detect the OSD Version */
error = __detect_osd(oud);
if (error) {
OSD_ERR("osd detection failed, non-compatible OSD device\n");
-   goto err_put_disk;
+   goto err_free_osd;
}
 
/* init the char-device for communication with user-mode */
cdev_init(>cdev, _fops);
oud->cdev.owner = THIS_MODULE;
-   error = cdev_add(>cdev,
-MKDEV(SCSI_OSD_MAJOR, oud->minor), 1);
-   if (error) {
-   OSD_ERR("cdev_add failed\n");
-   goto err_put_disk;
-   }
 
-   /* class device member */
-   oud->class_dev.devt = oud->cdev.dev;
-   oud->class_dev.class = _uld_class;
-   oud->class_dev.parent = dev;
-   oud->class_dev.release = __remove;
error = dev_set_name(>class_dev, "%s", disk->disk_name);
if (error) {
OSD_ERR("dev_set_name failed => %d\n", error);
-   goto err_put_cdev;
+   goto err_free_osd;
}
 
-   error = device_register(>class_dev);
+   error = cdev_device_add(>cdev, >class_dev);
if (error) {
OSD_ERR("device_register failed => %d\n", error);
-   goto err_put_cdev;
+   goto err_free_osd;
}
 
-   get_device(>class_dev);
-
OSD_INFO("osd_probe %s\n", disk->disk_name);
return 0;
 
-err_put_cdev:
-   cdev_del(>cdev);
-err_put_disk:
-   scsi_device_put(scsi_device);
-   put_disk(disk);
 err_free_osd:
-   dev_set_drvdata(dev, NULL);
-   kfree(oud);
+   put_device(>class_dev);
 err_retract_minor:
ida_remove(_minor_ida, minor);
return error;
@@ -530,9 +513,10 @@ static int osd_remove(struct device *dev)
scsi_device);
}
 
-   device_unregister(>class_dev);
-
+   cdev_device_del(>cdev, >class_dev);
+   ida_remove(_minor_ida, oud->minor);
put_device(>class_dev);
+
return 0;
 }
 
-- 
2.1.4

[PATCH v5 06/16] tpm-chip: utilize new cdev_device_add helper function

[Logan Gunthorpe]

Replace the open coded registration of the cdev and dev with the
new device_add_cdev() helper. The helper replaces a common pattern by
taking the proper reference against the parent device and adding both
the cdev and the device.

Signed-off-by: Logan Gunthorpe 
Reviewed-by: Jason Gunthorpe 
Reviewed-by: Jarkko Sakkinen 
---
 drivers/char/tpm/tpm-chip.c | 19 +++
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index c406343..935f0e9 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -187,7 +187,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 
cdev_init(>cdev, _fops);
chip->cdev.owner = THIS_MODULE;
-   chip->cdev.kobj.parent = >dev.kobj;
 
return chip;
 
@@ -230,27 +229,16 @@ static int tpm_add_char_device(struct tpm_chip *chip)
 {
int rc;
 
-   rc = cdev_add(>cdev, chip->dev.devt, 1);
+   rc = cdev_device_add(>cdev, >dev);
if (rc) {
dev_err(>dev,
-   "unable to cdev_add() %s, major %d, minor %d, err=%d\n",
+   "unable to cdev_device_add() %s, major %d, minor %d, 
err=%d\n",
dev_name(>dev), MAJOR(chip->dev.devt),
MINOR(chip->dev.devt), rc);
 
return rc;
}
 
-   rc = device_add(>dev);
-   if (rc) {
-   dev_err(>dev,
-   "unable to device_register() %s, major %d, minor %d, 
err=%d\n",
-   dev_name(>dev), MAJOR(chip->dev.devt),
-   MINOR(chip->dev.devt), rc);
-
-   cdev_del(>cdev);
-   return rc;
-   }
-
/* Make the chip available. */
mutex_lock(_lock);
idr_replace(_nums_idr, chip, chip->dev_num);
@@ -261,8 +249,7 @@ static int tpm_add_char_device(struct tpm_chip *chip)
 
 static void tpm_del_char_device(struct tpm_chip *chip)
 {
-   cdev_del(>cdev);
-   device_del(>dev);
+   cdev_device_del(>cdev, >dev);
 
/* Make the chip unavailable. */
mutex_lock(_lock);
-- 
2.1.4

[PATCH v5 00/16] Cleanup chardev instances with helper function

[Logan Gunthorpe]

Hey,

This version of the series fixes the issue found by the kbuild test
robot with the rtc driver. I managed to reproduce the issue and this
series fixes the problem.

Logan


Changes since v4:

* Fix a kbuild robot issue with the rtc driver: the rtc driver sometimes
  does not want to add the cdev. In order to accommodate this, the new
  cdev_device helper functions check dev->devt and if it's zero they
  don't add or delete the cdev.

* Remove prototypes for functions that were removed in the rtc driver

Changes since v3:

* Added a missing "device.h" include which caused warnings with some
  build configurations

Changes since v2:

* Expanded comments as per Jason's suggestions
* Collected tags
* Updated the switchtec patch seeing it's underlying patch set changed

Changes since v1:

* Expanded the idea to take care of adding the cdev and the device

Logan


Dan Williams (1):
  device-dax: fix cdev leak

Jason Gunthorpe (1):
  IB/ucm: utilize new cdev_device_add helper function

Logan Gunthorpe (14):
  chardev: add helper function to register char devs with a struct
device
  device-dax: utilize new cdev_device_add helper function
  input: utilize new cdev_device_add helper function
  gpiolib: utilize new cdev_device_add helper function
  tpm-chip: utilize new cdev_device_add helper function
  platform/chrome: cros_ec_dev - utilize new cdev_device_add helper
function
  infiniband: utilize the new cdev_set_parent function
  iio:core: utilize new cdev_device_add helper function
  media: utilize new cdev_device_add helper function
  mtd: utilize new cdev_device_add helper function
  rapidio: utilize new cdev_device_add helper function
  rtc: utilize new cdev_device_add helper function
  scsi: utilize new cdev_device_add helper function
  switchtec: utilize new device_add_cdev helper function

 drivers/char/tpm/tpm-chip.c  | 19 ++-
 drivers/dax/dax.c| 33 ++--
 drivers/gpio/gpiolib.c   | 23 +++-
 drivers/iio/industrialio-core.c  | 15 ++
 drivers/infiniband/core/ucm.c| 35 ++--
 drivers/infiniband/core/user_mad.c   |  4 +-
 drivers/infiniband/core/uverbs_main.c|  2 +-
 drivers/infiniband/hw/hfi1/device.c  |  2 +-
 drivers/input/evdev.c| 11 +---
 drivers/input/joydev.c   | 11 +---
 drivers/input/mousedev.c | 11 +---
 drivers/media/cec/cec-core.c | 16 ++
 drivers/media/media-devnode.c| 20 ++-
 drivers/mtd/ubi/build.c  | 91 ++--
 drivers/mtd/ubi/vmt.c| 49 ++---
 drivers/pci/switch/switchtec.c   | 11 +---
 drivers/platform/chrome/cros_ec_dev.c| 31 +++
 drivers/rapidio/devices/rio_mport_cdev.c | 24 +++--
 drivers/rtc/class.c  | 14 +++--
 drivers/rtc/rtc-core.h   | 10 
 drivers/rtc/rtc-dev.c| 17 --
 drivers/scsi/osd/osd_uld.c   | 56 +++-
 fs/char_dev.c| 86 ++
 include/linux/cdev.h |  5 ++
 24 files changed, 239 insertions(+), 357 deletions(-)

--
2.1.4

[PATCH v5 03/16] device-dax: utilize new cdev_device_add helper function

[Logan Gunthorpe]

Replace the open coded registration of the cdev and dev with the
new device_add_cdev() helper. The helper replaces a common pattern by
taking the proper reference against the parent device and adding both
the cdev and the device.

Signed-off-by: Logan Gunthorpe 
Reviewed-by: Dan Williams 
Reviewed-by: Johannes Thumshirn 
---
 drivers/dax/dax.c | 18 +-
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
index a5ed59a..e5f8555 100644
--- a/drivers/dax/dax.c
+++ b/drivers/dax/dax.c
@@ -677,8 +677,6 @@ static void dax_dev_release(struct device *dev)
 
 static void kill_dax_dev(struct dax_dev *dax_dev)
 {
-   struct cdev *cdev = _dev->cdev;
-
/*
 * Note, rcu is not protecting the liveness of dax_dev, rcu is
 * ensuring that any fault handlers that might have seen
@@ -689,7 +687,6 @@ static void kill_dax_dev(struct dax_dev *dax_dev)
dax_dev->alive = false;
synchronize_rcu();
unmap_mapping_range(dax_dev->inode->i_mapping, 0, 0, 1);
-   cdev_del(cdev);
 }
 
 static void unregister_dax_dev(void *dev)
@@ -699,7 +696,8 @@ static void unregister_dax_dev(void *dev)
dev_dbg(dev, "%s\n", __func__);
 
kill_dax_dev(dax_dev);
-   device_unregister(dev);
+   cdev_device_del(_dev->cdev, dev);
+   put_device(dev);
 }
 
 struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
@@ -750,18 +748,13 @@ struct dax_dev *devm_create_dax_dev(struct dax_region 
*dax_region,
goto err_inode;
}
 
-   /* device_initialize() so cdev can reference kobj parent */
+   /* from here on we're committed to teardown via dax_dev_release() */
device_initialize(dev);
 
cdev = _dev->cdev;
cdev_init(cdev, _fops);
cdev->owner = parent->driver->owner;
-   cdev->kobj.parent = >kobj;
-   rc = cdev_add(_dev->cdev, dev_t, 1);
-   if (rc)
-   goto err_cdev;
 
-   /* from here on we're committed to teardown via dax_dev_release() */
dax_dev->num_resources = count;
dax_dev->alive = true;
dax_dev->region = dax_region;
@@ -773,7 +766,8 @@ struct dax_dev *devm_create_dax_dev(struct dax_region 
*dax_region,
dev->groups = dax_attribute_groups;
dev->release = dax_dev_release;
dev_set_name(dev, "dax%d.%d", dax_region->id, dax_dev->id);
-   rc = device_add(dev);
+
+   rc = cdev_device_add(cdev, dev);
if (rc) {
kill_dax_dev(dax_dev);
put_device(dev);
@@ -786,8 +780,6 @@ struct dax_dev *devm_create_dax_dev(struct dax_region 
*dax_region,
 
return dax_dev;
 
- err_cdev:
-   iput(dax_dev->inode);
  err_inode:
ida_simple_remove(_minor_ida, minor);
  err_minor:
-- 
2.1.4

[PATCH v5 01/16] chardev: add helper function to register char devs with a struct device

[Logan Gunthorpe]

Credit for this patch goes is shared with Dan Williams [1]. I've
taken things one step further to make the helper function more
useful and clean up calling code.

There's a common pattern in the kernel whereby a struct cdev is placed
in a structure along side a struct device which manages the life-cycle
of both. In the naive approach, the reference counting is broken and
the struct device can free everything before the chardev code
is entirely released.

Many developers have solved this problem by linking the internal kobjs
in this fashion:

cdev.kobj.parent = _dev.kobj;

The cdev code explicitly gets and puts a reference to it's kobj parent.
So this seems like it was intended to be used this way. Dmitrty Torokhov
first put this in place in 2012 with this commit:

2f0157f char_dev: pin parent kobject

and the first instance of the fix was then done in the input subsystem
in the following commit:

4a215aa Input: fix use-after-free introduced with dynamic minor changes

Subsequently over the years, however, this issue seems to have tripped
up multiple developers independently. For example, see these commits:

0d5b7da iio: Prevent race between IIO chardev opening and IIO device
(by Lars-Peter Clausen in 2013)

ba0ef85 tpm: Fix initialization of the cdev
(by Jason Gunthorpe in 2015)

5b28dde [media] media: fix use-after-free in cdev_put() when app exits
after driver unbind
(by Shauh Khan in 2016)

This technique is similarly done in at least 15 places within the kernel
and probably should have been done so in another, at least, 5 places.
The kobj line also looks very suspect in that one would not expect
drivers to have to mess with kobject internals in this way.
Even highly experienced kernel developers can be surprised by this
code, as seen in [2].

To help alleviate this situation, and hopefully prevent future
wasted effort on this problem, this patch introduces a helper function
to register a char device along with its parent struct device.
This creates a more regular API for tying a char device to its parent
without the developer having to set members in the underlying kobject.

This patch introduce cdev_device_add and cdev_device_del which
replaces a common pattern including setting the kobj parent, calling
cdev_add and then calling device_add. It also introduces cdev_set_parent
for the few cases that set the kobject parent without using device_add.

[1] https://lkml.org/lkml/2017/2/13/700
[2] https://lkml.org/lkml/2017/2/10/370

Signed-off-by: Logan Gunthorpe 
Signed-off-by: Dan Williams 
Reviewed-by: Hans Verkuil 
Reviewed-by: Alexandre Belloni 
---
 fs/char_dev.c| 86 
 include/linux/cdev.h |  5 +++
 2 files changed, 91 insertions(+)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 44a240c..fb8507f 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -471,6 +471,85 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count)
return 0;
 }
 
+/**
+ * cdev_set_parent() - set the parent kobject for a char device
+ * @p: the cdev structure
+ * @kobj: the kobject to take a reference to
+ *
+ * cdev_set_parent() sets a parent kobject which will be referenced
+ * appropriately so the parent is not freed before the cdev. This
+ * should be called before cdev_add.
+ */
+void cdev_set_parent(struct cdev *p, struct kobject *kobj)
+{
+   WARN_ON(!kobj->state_initialized);
+   p->kobj.parent = kobj;
+}
+
+/**
+ * cdev_device_add() - add a char device and it's corresponding
+ * struct device, linkink
+ * @dev: the device structure
+ * @cdev: the cdev structure
+ *
+ * cdev_device_add() adds the char device represented by @cdev to the system,
+ * just as cdev_add does. It then adds @dev to the system using device_add
+ * The dev_t for the char device will be taken from the struct device which
+ * needs to be initialized first. This helper function correctly takes a
+ * reference to the parent device so the parent will not get released until
+ * all references to the cdev are released.
+ *
+ * This helper uses dev->devt for the device number. If it is not set
+ * it will not add the cdev and it will be equivalent to device_add.
+ *
+ * This function should be used whenever the struct cdev and the
+ * struct device are members of the same structure whose lifetime is
+ * managed by the struct device.
+ *
+ * NOTE: Callers must assume that userspace was able to open the cdev and
+ * can call cdev fops callbacks at any time, even if this function fails.
+ */
+int cdev_device_add(struct cdev *cdev, struct device *dev)
+{
+   int rc = 0;
+
+   if (dev->devt) {
+   cdev_set_parent(cdev, >kobj);
+
+   rc = cdev_add(cdev, dev->devt, 1);
+   if (rc)
+   return rc;
+   }
+
+   rc = device_add(dev);
+   if (rc)
+   cdev_del(cdev);
+
+   return rc;
+}
+
+/**
+ 

[PATCH v5 02/16] device-dax: fix cdev leak

[Logan Gunthorpe]

From: Dan Williams 

If device_add() fails, cleanup the cdev. Otherwise, we leak a kobj_map()
with a stale device number.

As Jason points out, there is a small possibility that userspace has
opened and mapped the device in the time between cdev_add() and the
device_add() failure. We need a new kill_dax_dev() helper to invalidate
any established mappings.

Fixes: ba09c01d2fa8 ("dax: convert to the cdev api")
Cc: 
Reported-by: Jason Gunthorpe 
Signed-off-by: Dan Williams 
Signed-off-by: Logan Gunthorpe 
Reviewed-by: Johannes Thumshirn 
---
 drivers/dax/dax.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
index 8d9829f..a5ed59a 100644
--- a/drivers/dax/dax.c
+++ b/drivers/dax/dax.c
@@ -675,13 +675,10 @@ static void dax_dev_release(struct device *dev)
kfree(dax_dev);
 }
 
-static void unregister_dax_dev(void *dev)
+static void kill_dax_dev(struct dax_dev *dax_dev)
 {
-   struct dax_dev *dax_dev = to_dax_dev(dev);
struct cdev *cdev = _dev->cdev;
 
-   dev_dbg(dev, "%s\n", __func__);
-
/*
 * Note, rcu is not protecting the liveness of dax_dev, rcu is
 * ensuring that any fault handlers that might have seen
@@ -693,6 +690,15 @@ static void unregister_dax_dev(void *dev)
synchronize_rcu();
unmap_mapping_range(dax_dev->inode->i_mapping, 0, 0, 1);
cdev_del(cdev);
+}
+
+static void unregister_dax_dev(void *dev)
+{
+   struct dax_dev *dax_dev = to_dax_dev(dev);
+
+   dev_dbg(dev, "%s\n", __func__);
+
+   kill_dax_dev(dax_dev);
device_unregister(dev);
 }
 
@@ -769,6 +775,7 @@ struct dax_dev *devm_create_dax_dev(struct dax_region 
*dax_region,
dev_set_name(dev, "dax%d.%d", dax_region->id, dax_dev->id);
rc = device_add(dev);
if (rc) {
+   kill_dax_dev(dax_dev);
put_device(dev);
return ERR_PTR(rc);
}
-- 
2.1.4

[PATCH v5 04/16] input: utilize new cdev_device_add helper function

[Logan Gunthorpe]

Replace the open coded registration of the cdev and dev with the
new device_add_cdev() helper in evdev, joydev and mousedev. The helper
replaces a common pattern by taking the proper reference against the
parent device and adding both the cdev and the device.

Signed-off-by: Logan Gunthorpe 
Acked-by: Dmitry Torokhov 
---
 drivers/input/evdev.c| 11 ++-
 drivers/input/joydev.c   | 11 ++-
 drivers/input/mousedev.c | 11 ++-
 3 files changed, 6 insertions(+), 27 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..9255714 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -1354,8 +1354,6 @@ static void evdev_cleanup(struct evdev *evdev)
evdev_mark_dead(evdev);
evdev_hangup(evdev);
 
-   cdev_del(>cdev);
-
/* evdev is marked dead so no one else accesses evdev->open */
if (evdev->open) {
input_flush_device(handle, NULL);
@@ -1416,12 +1414,8 @@ static int evdev_connect(struct input_handler *handler, 
struct input_dev *dev,
goto err_free_evdev;
 
cdev_init(>cdev, _fops);
-   evdev->cdev.kobj.parent = >dev.kobj;
-   error = cdev_add(>cdev, evdev->dev.devt, 1);
-   if (error)
-   goto err_unregister_handle;
 
-   error = device_add(>dev);
+   error = cdev_device_add(>cdev, >dev);
if (error)
goto err_cleanup_evdev;
 
@@ -1429,7 +1423,6 @@ static int evdev_connect(struct input_handler *handler, 
struct input_dev *dev,
 
  err_cleanup_evdev:
evdev_cleanup(evdev);
- err_unregister_handle:
input_unregister_handle(>handle);
  err_free_evdev:
put_device(>dev);
@@ -1442,7 +1435,7 @@ static void evdev_disconnect(struct input_handle *handle)
 {
struct evdev *evdev = handle->private;
 
-   device_del(>dev);
+   cdev_device_del(>cdev, >dev);
evdev_cleanup(evdev);
input_free_minor(MINOR(evdev->dev.devt));
input_unregister_handle(handle);
diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 065e67b..29d677c 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -742,8 +742,6 @@ static void joydev_cleanup(struct joydev *joydev)
joydev_mark_dead(joydev);
joydev_hangup(joydev);
 
-   cdev_del(>cdev);
-
/* joydev is marked dead so no one else accesses joydev->open */
if (joydev->open)
input_close_device(handle);
@@ -913,12 +911,8 @@ static int joydev_connect(struct input_handler *handler, 
struct input_dev *dev,
goto err_free_joydev;
 
cdev_init(>cdev, _fops);
-   joydev->cdev.kobj.parent = >dev.kobj;
-   error = cdev_add(>cdev, joydev->dev.devt, 1);
-   if (error)
-   goto err_unregister_handle;
 
-   error = device_add(>dev);
+   error = cdev_device_add(>cdev, >dev);
if (error)
goto err_cleanup_joydev;
 
@@ -926,7 +920,6 @@ static int joydev_connect(struct input_handler *handler, 
struct input_dev *dev,
 
  err_cleanup_joydev:
joydev_cleanup(joydev);
- err_unregister_handle:
input_unregister_handle(>handle);
  err_free_joydev:
put_device(>dev);
@@ -939,7 +932,7 @@ static void joydev_disconnect(struct input_handle *handle)
 {
struct joydev *joydev = handle->private;
 
-   device_del(>dev);
+   cdev_device_del(>cdev, >dev);
joydev_cleanup(joydev);
input_free_minor(MINOR(joydev->dev.devt));
input_unregister_handle(handle);
diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c
index b604564..0e0ff84 100644
--- a/drivers/input/mousedev.c
+++ b/drivers/input/mousedev.c
@@ -812,8 +812,6 @@ static void mousedev_cleanup(struct mousedev *mousedev)
mousedev_mark_dead(mousedev);
mousedev_hangup(mousedev);
 
-   cdev_del(>cdev);
-
/* mousedev is marked dead so no one else accesses mousedev->open */
if (mousedev->open)
input_close_device(handle);
@@ -901,12 +899,8 @@ static struct mousedev *mousedev_create(struct input_dev 
*dev,
}
 
cdev_init(>cdev, _fops);
-   mousedev->cdev.kobj.parent = >dev.kobj;
-   error = cdev_add(>cdev, mousedev->dev.devt, 1);
-   if (error)
-   goto err_unregister_handle;
 
-   error = device_add(>dev);
+   error = cdev_device_add(>cdev, >dev);
if (error)
goto err_cleanup_mousedev;
 
@@ -914,7 +908,6 @@ static struct mousedev *mousedev_create(struct input_dev 
*dev,
 
  err_cleanup_mousedev:
mousedev_cleanup(mousedev);
- err_unregister_handle:
if (!mixdev)
input_unregister_handle(>handle);
  err_free_mousedev:
@@ -927,7 +920,7 @@ static struct mousedev *mousedev_create(struct input_dev 
*dev,
 
 static void mousedev_destroy(struct mousedev *mousedev)
 {
-   device_del(>dev);
+   cdev_device_del(>cdev, >dev);

[PATCH v5 08/16] IB/ucm: utilize new cdev_device_add helper function

[Logan Gunthorpe]

From: Jason Gunthorpe 

The use after free is not triggerable here because the cdev holds
the module lock and the only device_unregister is only triggered by
module unload, however make the change for consistency.

To make this work the cdev_del needs to move out of the struct device
release function.

This cleans up the error path significantly and thus also fixes a minor
bug where the devnum would not be released if cdev_add failed.

Signed-off-by: Jason Gunthorpe 
Signed-off-by: Logan Gunthorpe 
Reviewed-by: Logan Gunthorpe 
Reviewed-by: Leon Romanovsky 
---
 drivers/infiniband/core/ucm.c | 35 ++-
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c
index cc0d51f..d15efa4 100644
--- a/drivers/infiniband/core/ucm.c
+++ b/drivers/infiniband/core/ucm.c
@@ -1205,12 +1205,15 @@ static void ib_ucm_release_dev(struct device *dev)
struct ib_ucm_device *ucm_dev;
 
ucm_dev = container_of(dev, struct ib_ucm_device, dev);
-   cdev_del(_dev->cdev);
+   kfree(ucm_dev);
+}
+
+static void ib_ucm_free_dev(struct ib_ucm_device *ucm_dev)
+{
if (ucm_dev->devnum < IB_UCM_MAX_DEVICES)
clear_bit(ucm_dev->devnum, dev_map);
else
clear_bit(ucm_dev->devnum - IB_UCM_MAX_DEVICES, overflow_map);
-   kfree(ucm_dev);
 }
 
 static const struct file_operations ucm_fops = {
@@ -1266,7 +1269,9 @@ static void ib_ucm_add_one(struct ib_device *device)
if (!ucm_dev)
return;
 
+   device_initialize(_dev->dev);
ucm_dev->ib_dev = device;
+   ucm_dev->dev.release = ib_ucm_release_dev;
 
devnum = find_first_zero_bit(dev_map, IB_UCM_MAX_DEVICES);
if (devnum >= IB_UCM_MAX_DEVICES) {
@@ -1286,16 +1291,14 @@ static void ib_ucm_add_one(struct ib_device *device)
cdev_init(_dev->cdev, _fops);
ucm_dev->cdev.owner = THIS_MODULE;
kobject_set_name(_dev->cdev.kobj, "ucm%d", ucm_dev->devnum);
-   if (cdev_add(_dev->cdev, base, 1))
-   goto err;
 
ucm_dev->dev.class = _class;
ucm_dev->dev.parent = device->dev.parent;
-   ucm_dev->dev.devt = ucm_dev->cdev.dev;
-   ucm_dev->dev.release = ib_ucm_release_dev;
+   ucm_dev->dev.devt = base;
+
dev_set_name(_dev->dev, "ucm%d", ucm_dev->devnum);
-   if (device_register(_dev->dev))
-   goto err_cdev;
+   if (cdev_device_add(_dev->cdev, _dev->dev))
+   goto err_devnum;
 
if (device_create_file(_dev->dev, _attr_ibdev))
goto err_dev;
@@ -1304,15 +1307,11 @@ static void ib_ucm_add_one(struct ib_device *device)
return;
 
 err_dev:
-   device_unregister(_dev->dev);
-err_cdev:
-   cdev_del(_dev->cdev);
-   if (ucm_dev->devnum < IB_UCM_MAX_DEVICES)
-   clear_bit(devnum, dev_map);
-   else
-   clear_bit(devnum, overflow_map);
+   cdev_device_del(_dev->cdev, _dev->dev);
+err_devnum:
+   ib_ucm_free_dev(ucm_dev);
 err:
-   kfree(ucm_dev);
+   put_device(_dev->dev);
return;
 }
 
@@ -1323,7 +1322,9 @@ static void ib_ucm_remove_one(struct ib_device *device, 
void *client_data)
if (!ucm_dev)
return;
 
-   device_unregister(_dev->dev);
+   cdev_device_del(_dev->cdev, _dev->dev);
+   ib_ucm_free_dev(ucm_dev);
+   put_device(_dev->dev);
 }
 
 static CLASS_ATTR_STRING(abi_version, S_IRUGO,
-- 
2.1.4

[PATCH v5 16/16] switchtec: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Very straightforward conversion to device_add_cdev. Drop cdev_add and
device_add and use cdev_device_add.

Signed-off-by: Logan Gunthorpe 
---
 drivers/pci/switch/switchtec.c | 11 ++-
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index 1f045c9..e27fd29 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -1291,7 +1291,6 @@ static struct switchtec_dev *stdev_create(struct pci_dev 
*pdev)
cdev = >cdev;
cdev_init(cdev, _fops);
cdev->owner = THIS_MODULE;
-   cdev->kobj.parent = >kobj;
 
return stdev;
 
@@ -1479,11 +1478,7 @@ static int switchtec_pci_probe(struct pci_dev *pdev,
  SWITCHTEC_EVENT_EN_IRQ,
  >mmio_part_cfg->mrpc_comp_hdr);
 
-   rc = cdev_add(>cdev, stdev->dev.devt, 1);
-   if (rc)
-   goto err_put;
-
-   rc = device_add(>dev);
+   rc = cdev_device_add(>cdev, >dev);
if (rc)
goto err_devadd;
 
@@ -1492,7 +1487,6 @@ static int switchtec_pci_probe(struct pci_dev *pdev,
return 0;
 
 err_devadd:
-   cdev_del(>cdev);
stdev_kill(stdev);
 err_put:
ida_simple_remove(_minor_ida, MINOR(stdev->dev.devt));
@@ -1506,8 +1500,7 @@ static void switchtec_pci_remove(struct pci_dev *pdev)
 
pci_set_drvdata(pdev, NULL);
 
-   device_del(>dev);
-   cdev_del(>cdev);
+   cdev_device_del(>cdev, >dev);
ida_simple_remove(_minor_ida, MINOR(stdev->dev.devt));
dev_info(>dev, "unregistered.\n");
 
-- 
2.1.4

[PATCH v5 14/16] rtc: utilize new cdev_device_add helper function

[Logan Gunthorpe]

Mostly straightforward, but we had to remove the rtc_dev_add/del_device
functions as they split up the cdev_add and the device_add.

Doing this also revealed that there was likely another subtle bug:
seeing cdev_add was done after device_register, the cdev probably
was not ready before device_add when the uevent occurs. This would
race with userspace, if it tried to use the device directly after
the uevent. This is fixed just by using the new helper function.

Another weird thing is this driver would, in some error cases, call
cdev_add() without calling cdev_init. This patchset corrects this
by avoiding calling cdev_add if the devt is not set.

Signed-off-by: Logan Gunthorpe 
Acked-by: Alexandre Belloni 
---
 drivers/rtc/class.c| 14 ++
 drivers/rtc/rtc-core.h | 10 --
 drivers/rtc/rtc-dev.c  | 17 -
 3 files changed, 10 insertions(+), 31 deletions(-)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 74fd974..5fb4398 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -195,6 +195,8 @@ struct rtc_device *rtc_device_register(const char *name, 
struct device *dev,
goto exit_ida;
}
 
+   device_initialize(>dev);
+
rtc->id = id;
rtc->ops = ops;
rtc->owner = owner;
@@ -233,14 +235,19 @@ struct rtc_device *rtc_device_register(const char *name, 
struct device *dev,
 
rtc_dev_prepare(rtc);
 
-   err = device_register(>dev);
+   err = cdev_device_add(>char_dev, >dev);
if (err) {
+   dev_warn(>dev, "%s: failed to add char device %d:%d\n",
+rtc->name, MAJOR(rtc->dev.devt), rtc->id);
+
/* This will free both memory and the ID */
put_device(>dev);
goto exit;
+   } else {
+   dev_dbg(>dev, "%s: dev (%d:%d)\n", rtc->name,
+   MAJOR(rtc->dev.devt), rtc->id);
}
 
-   rtc_dev_add_device(rtc);
rtc_proc_add_device(rtc);
 
dev_info(dev, "rtc core: registered %s as %s\n",
@@ -271,9 +278,8 @@ void rtc_device_unregister(struct rtc_device *rtc)
 * Remove innards of this RTC, then disable it, before
 * letting any rtc_class_open() users access it again
 */
-   rtc_dev_del_device(rtc);
rtc_proc_del_device(rtc);
-   device_del(>dev);
+   cdev_device_del(>char_dev, >dev);
rtc->ops = NULL;
mutex_unlock(>ops_lock);
put_device(>dev);
diff --git a/drivers/rtc/rtc-core.h b/drivers/rtc/rtc-core.h
index a098aea..7a4ed2f 100644
--- a/drivers/rtc/rtc-core.h
+++ b/drivers/rtc/rtc-core.h
@@ -3,8 +3,6 @@
 extern void __init rtc_dev_init(void);
 extern void __exit rtc_dev_exit(void);
 extern void rtc_dev_prepare(struct rtc_device *rtc);
-extern void rtc_dev_add_device(struct rtc_device *rtc);
-extern void rtc_dev_del_device(struct rtc_device *rtc);
 
 #else
 
@@ -20,14 +18,6 @@ static inline void rtc_dev_prepare(struct rtc_device *rtc)
 {
 }
 
-static inline void rtc_dev_add_device(struct rtc_device *rtc)
-{
-}
-
-static inline void rtc_dev_del_device(struct rtc_device *rtc)
-{
-}
-
 #endif
 
 #ifdef CONFIG_RTC_INTF_PROC
diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
index 6dc8f29..e81a871 100644
--- a/drivers/rtc/rtc-dev.c
+++ b/drivers/rtc/rtc-dev.c
@@ -477,23 +477,6 @@ void rtc_dev_prepare(struct rtc_device *rtc)
 
cdev_init(>char_dev, _dev_fops);
rtc->char_dev.owner = rtc->owner;
-   rtc->char_dev.kobj.parent = >dev.kobj;
-}
-
-void rtc_dev_add_device(struct rtc_device *rtc)
-{
-   if (cdev_add(>char_dev, rtc->dev.devt, 1))
-   dev_warn(>dev, "%s: failed to add char device %d:%d\n",
-   rtc->name, MAJOR(rtc_devt), rtc->id);
-   else
-   dev_dbg(>dev, "%s: dev (%d:%d)\n", rtc->name,
-   MAJOR(rtc_devt), rtc->id);
-}
-
-void rtc_dev_del_device(struct rtc_device *rtc)
-{
-   if (rtc->dev.devt)
-   cdev_del(>char_dev);
 }
 
 void __init rtc_dev_init(void)
-- 
2.1.4

[PATCH v5 09/16] infiniband: utilize the new cdev_set_parent function

[Logan Gunthorpe]

This replaces the suspect looking cdev.kobj.parent lines with the
equivalent cdev_set_parent function. This is a straightforward change
that's largely cosmetic but it does push the kobj.parent ownership
into char_dev.c where it belongs.

Signed-off-by: Logan Gunthorpe 
---
 drivers/infiniband/core/user_mad.c| 4 ++--
 drivers/infiniband/core/uverbs_main.c | 2 +-
 drivers/infiniband/hw/hfi1/device.c   | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/core/user_mad.c 
b/drivers/infiniband/core/user_mad.c
index aca7ff7..25071b8 100644
--- a/drivers/infiniband/core/user_mad.c
+++ b/drivers/infiniband/core/user_mad.c
@@ -1183,7 +1183,7 @@ static int ib_umad_init_port(struct ib_device *device, 
int port_num,
 
cdev_init(>cdev, _fops);
port->cdev.owner = THIS_MODULE;
-   port->cdev.kobj.parent = _dev->kobj;
+   cdev_set_parent(>cdev, _dev->kobj);
kobject_set_name(>cdev.kobj, "umad%d", port->dev_num);
if (cdev_add(>cdev, base, 1))
goto err_cdev;
@@ -1202,7 +1202,7 @@ static int ib_umad_init_port(struct ib_device *device, 
int port_num,
base += IB_UMAD_MAX_PORTS;
cdev_init(>sm_cdev, _sm_fops);
port->sm_cdev.owner = THIS_MODULE;
-   port->sm_cdev.kobj.parent = _dev->kobj;
+   cdev_set_parent(>sm_cdev, _dev->kobj);
kobject_set_name(>sm_cdev.kobj, "issm%d", port->dev_num);
if (cdev_add(>sm_cdev, base, 1))
goto err_sm_cdev;
diff --git a/drivers/infiniband/core/uverbs_main.c 
b/drivers/infiniband/core/uverbs_main.c
index 35c788a..f02d7b9 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -1189,7 +1189,7 @@ static void ib_uverbs_add_one(struct ib_device *device)
cdev_init(_dev->cdev, NULL);
uverbs_dev->cdev.owner = THIS_MODULE;
uverbs_dev->cdev.ops = device->mmap ? _mmap_fops : _fops;
-   uverbs_dev->cdev.kobj.parent = _dev->kobj;
+   cdev_set_parent(_dev->cdev, _dev->kobj);
kobject_set_name(_dev->cdev.kobj, "uverbs%d", 
uverbs_dev->devnum);
if (cdev_add(_dev->cdev, base, 1))
goto err_cdev;
diff --git a/drivers/infiniband/hw/hfi1/device.c 
b/drivers/infiniband/hw/hfi1/device.c
index bf64b5a..bbb6069 100644
--- a/drivers/infiniband/hw/hfi1/device.c
+++ b/drivers/infiniband/hw/hfi1/device.c
@@ -69,7 +69,7 @@ int hfi1_cdev_init(int minor, const char *name,
 
cdev_init(cdev, fops);
cdev->owner = THIS_MODULE;
-   cdev->kobj.parent = parent;
+   cdev_set_parent(cdev, parent);
kobject_set_name(>kobj, name);
 
ret = cdev_add(cdev, dev, 1);
-- 
2.1.4

Re: [PATCHv2 2/5] target/user: Add global data block pool support

[Andy Grover]

On 03/17/2017 01:04 AM, Xiubo Li wrote:

[...]

These days what I have gotten is that the unmap_mapping_range() could
be used.
At the same time I have deep into the mm code and fixed the double
usage of
the data blocks and possible page fault call trace bugs mentioned above.

Following is the V3 patch. I have test this using 4 targets & fio for
about 2 days, so
far so good.

I'm still testing this using more complex test case.


I have test it the whole day today:
- using 4 targets
- setting TCMU_GLOBAL_MAX_BLOCKS = [512 1K 1M 1G 2G]
- each target here needs more than 450 blocks when running
- fio: -iodepth [1 2 4 8 16] -thread -rw=[read write] -bs=[1K 2K 3K 5K
7K 16K 64K 1M] -size=20G -numjobs=10 -runtime=1000  ...


Hi Xiubo,

V3 is sounding very good. I look forward to reviewing it after it is posted.

Thanks -- Regards -- Andy

Re: [PATCH 0/3] Unblock SCSI devices even if the LLD is being unloaded

[Bart Van Assche]

On Fri, 2017-03-17 at 05:54 -0700, James Bottomley wrote:
> So it's better to use the module without a reference in place and take
> the risk that it may exit and release its code area while we're calling
> it?

Hello James,

My understanding of scsi_device_get() / scsi_device_put() is that the reason
why these manipulate the module reference count is to avoid that a SCSI LLD
module can be unloaded while a SCSI device is being used from a context that
is not notified about SCSI LLD unloading (e.g. a file handle controlled by
the sd driver or a SCSI ALUA device handler worker thread).

Does your comment mean that you think there is a scenario in which
scsi_target_block() or scsi_target_unblock() can be called while the text
area of a SCSI LLD is being released? I have reviewed all the callers of
these functions but I have not found such a scenario. scsi_target_block()
and scsi_target_unblock() are either called from a SCSI transport layer
implementation (FC, iSCSI, SRP) or from a SCSI LLD kernel module (snic_disc).
All these kernel modules only call scsi_target_*block() for resources (rport
or SCSI target respectively) that are removed before the code area of these
modules is released. This is why I think that calling scsi_target_*block()
without increasing the SCSI LLD module reference count is safe.

> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 82dfe07..fd1ba1d 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -39,6 +39,7 @@ static const struct {
>   { SDEV_TRANSPORT_OFFLINE, "transport-offline" },
>   { SDEV_BLOCK,   "blocked" },
>   { SDEV_CREATED_BLOCK, "created-blocked" },
> + { SDEV_CANCEL_BLOCK, "blocked" },
>  };

The multipathd function path_offline() translates "blocked" into PATH_PENDING.
Shouldn't SDEV_CANCEL_BLOCK be translated by multipathd into PATH_DOWN? There
might be other user space applications that interpret the SCSI device state
and that I am not aware of.

Additionally, your patch does not modify scsi_device_get() and hence will
cause scsi_device_get() to succeed for devices that are in state
SDEV_CANCEL_BLOCK. I think that's a subtle behavior change.

Thanks,

Bart.

[PATCH v2] scsi: storvsc: Add support for FC rport.

[Cathy Avery]

Included in the current storvsc driver for Hyper-V is the ability
to access luns on an FC fabric via a virtualized fiber channel
adapter exposed by the Hyper-V host. The driver also attaches to
the FC transport to allow host and port names to be published under
/sys/class/fc_host/hostX. Current customer tools running on the VM
require that these names be available in the well known standard
location under fc_host/hostX.

A problem arose when attaching to the FC transport. The scsi_scan
code attempts to call fc_user_scan which has basically become a no-op
due to the fact that the virtualized FC device does not expose rports.
At this point you cannot refresh the scsi bus after mapping or unmapping
luns on the SAN without a reboot of the VM.

This patch stubs in an rport per fc_host in storvsc so that the
requirement of a defined rport is now met within the fc_transport and
echo "- - -" > /sys/class/scsi_host/hostX/scan now works.

Signed-off-by: Cathy Avery 
---
Changes since v1:
- Fix fc_rport_identifiers init [Stephen Hemminger]
- Better error checking
---
 drivers/scsi/storvsc_drv.c | 23 ++-
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 638e5f4..37646d1 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -478,6 +478,9 @@ struct storvsc_device {
 */
u64 node_name;
u64 port_name;
+#if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
+   struct fc_rport *rport;
+#endif
 };
 
 struct hv_host_device {
@@ -1816,19 +1819,27 @@ static int storvsc_probe(struct hv_device *device,
target = (device->dev_instance.b[5] << 8 |
 device->dev_instance.b[4]);
ret = scsi_add_device(host, 0, target, 0);
-   if (ret) {
-   scsi_remove_host(host);
-   goto err_out2;
-   }
+   if (ret)
+   goto err_out3;
}
 #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
if (host->transportt == fc_transport_template) {
+   struct fc_rport_identifiers ids = {
+   .roles = FC_PORT_ROLE_FCP_TARGET,
+   };
+
fc_host_node_name(host) = stor_device->node_name;
fc_host_port_name(host) = stor_device->port_name;
+   stor_device->rport = fc_remote_port_add(host, 0, );
+   if (!stor_device->rport)
+   goto err_out3;
}
 #endif
return 0;
 
+err_out3:
+   scsi_remove_host(host);
+
 err_out2:
/*
 * Once we have connected with the host, we would need to
@@ -1854,8 +1865,10 @@ static int storvsc_remove(struct hv_device *dev)
struct Scsi_Host *host = stor_device->host;
 
 #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
-   if (host->transportt == fc_transport_template)
+   if (host->transportt == fc_transport_template) {
+   fc_remote_port_delete(stor_device->rport);
fc_remove_host(host);
+   }
 #endif
scsi_remove_host(host);
storvsc_dev_remove(dev);
-- 
2.5.0

Re: [PATCH 0/3] Unblock SCSI devices even if the LLD is being unloaded

[James Bottomley]

On Thu, 2017-03-16 at 23:19 +, Bart Van Assche wrote:
> On Thu, 2017-03-16 at 15:53 -0700, James Bottomley wrote:
> > On Thu, 2017-03-16 at 13:56 -0700, Bart Van Assche wrote:
> > > scsi_target_unblock() must unblock SCSI devices even if this
> > > function
> > > is called after unloading of the LLD that created these devices
> > > has
> > > started. This is necessary to prevent that __scsi_remove_device()
> > > hangs on the SYNCHRONIZE CACHE command issued by the sd driver
> > > during
> > > shutdown.
> > 
> > Your special get function misses the try_module_get().  But this is
> > all
> > really a bit ugly. Since the only problem is the SYNC CACHE 
> > triggered by device_del, isn't a better solution a new state:
> > SDEV_CANCEL_BLOCK.  This will make the device visible to 
> > scsi_get_device() and we can take it back from CANCEL_BLOCKED
> > ->CANCEL when the queue is unblocked.  I suspect we could also 
> > simply throw away the sync cache command when the device is blocked 
> > (the cache should destage naturally in the time it takes for the
> > device to be unblocked).
> 
> Hello James,
> 
> The purpose of this patch series is to make sure that unblock also 
> occurs after module unloading has started. My understanding of
> try_module_get() is that it fails once module unloading has started.
>  In other words, it is on purpose that try_module_get() is not
> called. From the kernel module
> code:
> 
> bool try_module_get(struct module *module)
> {
>   bool ret = true;
> 
>   if (module) {
>   preempt_disable();
>   /* Note: here, we can fail to get a reference */
>   if (likely(module_is_live(module) &&
>  atomic_inc_not_zero(>refcnt) != 0))
>   trace_module_get(module, _RET_IP_);
>   else
>   ret = false;
>   preempt_enable();
>   }
>   return ret;
> }
> 
> static inline int module_is_live(struct module *mod)
> {
>   return mod->state != MODULE_STATE_GOING;
> }

So it's better to use the module without a reference in place and take
the risk that it may exit and release its code area while we're calling
it?

> Regarding introducing a new device state: this is something I would 
> like to avoid. Any code that manipulates the SCSI device state is
> unnecessarily hard to modify because multiple types of state 
> information have been mixed up in a single state variable (blocked / 
> not blocked; created / running / cancel / offline). Additionally, the 
> SCSI device state is visible to user space.
> Adding a new SCSI device state could break existing user space
> applications.

I'm not sure that's a real concern for a new cancel state, but it's
addressable by not exposing the state to user space (it can just show
up as blocked)

> There is another problem with the introduction of the 
> SDEV_CANCEL_BLOCKED state: we do not want open() calls to succeed for 
> devices that are in the SDEV_DEL, SDEV_CANCEL nor for devices that 
> are in the SDEV_CANCEL_BLOCKED state. scsi_disk_get() in the sd 
> driver relies on scsi_device_get() to check the SCSI device state. If 
> scsi_device_get() would succeed for devices in the
> SDEV_CANCEL_BLOCKED state then an explicit check for that state would 
> have to be added in several users of scsi_device_get().

That really doesn't matter: getting a reference via open is a race. 
 Currently if you do it just before SDEV_CANCEL you end up in the same
position: a properly refcounted open device that can't send any
commands, so this doesn't add any new problems.

>  In other words, I think adding the SDEV_CANCEL_BLOCKED state would
> result in a much more complex and also harder to test patch.

Fine, here's what I thing it would look like; it seems a lot shorter
and simpler to me, but if you want to pursue your approach fixing the
race with module exit is a requirement.

James

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ba22866..b952a6a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1248,6 +1248,13 @@ scsi_prep_state_check(struct scsi_device *sdev, struct 
request *req)
"rejecting I/O to dead device\n");
ret = BLKPREP_KILL;
break;
+   case SDEV_CANCEL_BLOCK:
+   /*
+* silently kill the I/O: the only allowed thing
+* is a ULD remove one (like SYNC CACHE)
+*/
+   ret = BLKPREP_KILL;
+   break;
case SDEV_BLOCK:
case SDEV_CREATED_BLOCK:
ret = BLKPREP_DEFER;
@@ -2604,6 +2611,15 @@ scsi_device_set_state(struct scsi_device *sdev, enum 
scsi_device_state state)
}
break;
 
+   case SDEV_CANCEL_BLOCK:
+   switch (oldstate) {
+   case SDEV_CANCEL:
+  

Could you please let me know if you receive my last email, introducing myself to you, on a ( Business Opportunity)? I urgently wish to know if we can work together.

[United Bank For Africa]

[PATCH v2] lsscsi: Fix truncation of 128-bit wwn

[Vaibhav Jain]

Currently with '--wwn' flag, 128-bit wwns gets truncated and their
last 3 hex-digits missing. Below is a comparison of wwn reported by
lsscsi compared to wwn info at /dev/disk/by-id directory.

% lsscsi -w 0:0:5:0
[0:0:5:0]disk0x60050764008181941  /dev/sdad

% ls -l /dev/disk/by-id/wwn-*
lrwxrwxrwx. 1 root root 10 Oct 19 01:08 
/dev/disk/by-id/wwn-0x600507640081819411b1 -> ../../sdad

To fix this, the patch increases the size of member wwn of struct
disk_wwn_node_entry to 35 chars to accommodate the extra '0x' prefix and
null terminator. Also the size of the buffer wwn_str thats used to
output wwn to the std-out is increased to match the corresponding
member of disk_wwn_node_entry.

Link: https://bugs.launchpad.net/ubuntu/+source/lsscsi/+bug/1636467
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1387263

Cc: Jon Grimm 
Cc: Vipin K Parashar 
Cc: Ping Tian Han 
Cc: Gris Ge 
Reported-by: Ping Tian Han 
Signed-off-by: Vaibhav Jain 
---
Change-log:

v2..v1
- Introduces macro DISK_WWN_MAX_LEN that defines the maximum length of
  wwn read from devfs. (Gris Ge)
- Fix the indentation using space instead of '\t' (Gris Ge)
---
 src/lsscsi.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/lsscsi.c b/src/lsscsi.c
index 974b3f1..5d3c602 100644
--- a/src/lsscsi.c
+++ b/src/lsscsi.c
@@ -209,8 +209,17 @@ struct dev_node_list {
 };
 static struct dev_node_list* dev_node_listhead = NULL;
 
+/* WWN here is extracted from /dev/disk/by-id/wwn- which is
+ * created by udev 60-persistent-storage.rules using ID_WWN_WITH_EXTENSION.
+ * The udev ID_WWN_WITH_EXTENSION is the combination of char wwn[17] and
+ * char wwn_vendor_extension[17] from struct scsi_id_device. This macro
+ * defines the maximum length of char-array needed to store this wwn including
+ * the null-terminator.
+ */
+#define DISK_WWN_MAX_LEN 35
+
 struct disk_wwn_node_entry {
-   char wwn[32];
+   char wwn[DISK_WWN_MAX_LEN]; /* '0x' + wwn<128-bit> +  
*/
char disk_bname[12];
 };
 
@@ -2939,14 +2948,15 @@ one_sdev_entry(const char * dir_name, const char * 
devname,
 }
 if (wd[0]) {
 char dev_node[LMAX_NAME] = "";
-char wwn_str[34];
+char wwn_str[DISK_WWN_MAX_LEN];
 enum dev_type typ;
 
 typ = (FT_BLOCK == non_sg.ft) ? BLK_DEV : CHR_DEV;
 if (get_wwn) {
 if ((BLK_DEV == typ) &&
 get_disk_wwn(wd, wwn_str, sizeof(wwn_str)))
-printf("%-30s  ", wwn_str);
+printf("%-*s  ", DISK_WWN_MAX_LEN - 1,
+   wwn_str);
 else
 printf("  "
"  ");
-- 
2.9.3

Re: [PATCH] lsscsi: Fix truncation of 128-bit wwn

[Gris Ge]

On Fri, Mar 17, 2017 at 10:08:40AM +0530, Vaibhav Jain wrote:
> ---
>  src/lsscsi.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/lsscsi.c b/src/lsscsi.c
> index 974b3f1..f20adcf 100644
> --- a/src/lsscsi.c
> +++ b/src/lsscsi.c
> @@ -210,8 +210,8 @@ struct dev_node_list {
>  static struct dev_node_list* dev_node_listhead = NULL;
>  
>  struct disk_wwn_node_entry {
> -   char wwn[32];
> -   char disk_bname[12];
> + char wwn[35]; /* '0x' + wwn<128-bit> +  */
Please consider to define a constant instead of using a magic number
everywhere. Like:
#define _DISK_WWN_MAX_LEN   35
/* ^ WWN here is extracted from /dev/disk/by-id/wwn- which is
 *   created by udev 60-persistent-storage.rules using ID_WWN_WITH_EXTENSION.
 *   The udev ID_WWN_WITH_EXTENSION is the combination of char wwn[17] and
 *   char wwn_vendor_extension[17] from struct scsi_id_device.
 */
> + char disk_bname[12];
Please use space instead of \t for indention.
>  };
>  
>  #define DISK_WWN_NODE_LIST_ENTRIES 16
> @@ -2939,14 +2939,14 @@ one_sdev_entry(const char * dir_name, const char * 
> devname,
>  }
>  if (wd[0]) {
>  char dev_node[LMAX_NAME] = "";
> -char wwn_str[34];
> + char wwn_str[35];
Please use space instead of \t for indention.
>  enum dev_type typ;
>  
>  typ = (FT_BLOCK == non_sg.ft) ? BLK_DEV : CHR_DEV;
>  if (get_wwn) {
>  if ((BLK_DEV == typ) &&
>  get_disk_wwn(wd, wwn_str, 
> sizeof(wwn_str)))
> -printf("%-30s  ", wwn_str);
> + printf("%-34s  ", wwn_str);
Please use constant instead of magic number:
printf("%-*s  ", _DISK_WWN_MAX_LEN - 1, wwn_str);
>  else
>  printf("  "
> "  ");
> -- 
> 2.9.3
> 

-- 
Gris Ge


signature.asc
Description: PGP signature

Re: [patch] check length passed to SG_NEXT_CMD_LEN

[Dmitry Vyukov]

On Fri, Mar 17, 2017 at 12:48 AM, Martin K. Petersen
 wrote:
> Peter Chang  writes:
>
> Applied to 4.11/scsi-fixes.
>
> Thanks!
>
> --
> Martin K. Petersen  Oracle Linux Engineering

Hi,

Can you point to the commit/tree? I don't see it here:
https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/log/?h=4.11/scsi-fixes

[PATCH] lsscsi: Fix truncation of 128-bit wwn

[Vaibhav Jain]

Currently with '--wwn' flag, 128-bit wwns gets truncated and their
last 3 hex-digits missing. Below is a comparison of wwn reported by
lsscsi compared to wwn info at /dev/disk/by-id directory.

% lsscsi -w 0:0:5:0
[0:0:5:0]disk0x60050764008181941  /dev/sdad

% ls -l /dev/disk/by-id/wwn-*
lrwxrwxrwx. 1 root root 10 Oct 19 01:08 
/dev/disk/by-id/wwn-0x600507640081819411b1 -> ../../sdad

To fix this, the patch increases the size of member wwn of struct
disk_wwn_node_entry to 35 chars to accommodate the extra '0x' prefix and
null terminator. Also the size of the buffer wwn_str thats used to
output wwn to the std-out is increased to match the corresponding
member of disk_wwn_node_entry.

Link: https://bugs.launchpad.net/ubuntu/+source/lsscsi/+bug/1636467
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1387263

Cc: Jon Grimm 
Cc: Vipin K Parashar 
Cc: Ping Tian Han 
Cc: Gris Ge 
Reported-by: Ping Tian Han 
Signed-off-by: Vaibhav Jain 
Reviewed-by: Gris Ge 
---
 src/lsscsi.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/lsscsi.c b/src/lsscsi.c
index 974b3f1..f20adcf 100644
--- a/src/lsscsi.c
+++ b/src/lsscsi.c
@@ -210,8 +210,8 @@ struct dev_node_list {
 static struct dev_node_list* dev_node_listhead = NULL;
 
 struct disk_wwn_node_entry {
-   char wwn[32];
-   char disk_bname[12];
+   char wwn[35]; /* '0x' + wwn<128-bit> +  */
+   char disk_bname[12];
 };
 
 #define DISK_WWN_NODE_LIST_ENTRIES 16
@@ -2939,14 +2939,14 @@ one_sdev_entry(const char * dir_name, const char * 
devname,
 }
 if (wd[0]) {
 char dev_node[LMAX_NAME] = "";
-char wwn_str[34];
+   char wwn_str[35];
 enum dev_type typ;
 
 typ = (FT_BLOCK == non_sg.ft) ? BLK_DEV : CHR_DEV;
 if (get_wwn) {
 if ((BLK_DEV == typ) &&
 get_disk_wwn(wd, wwn_str, sizeof(wwn_str)))
-printf("%-30s  ", wwn_str);
+   printf("%-34s  ", wwn_str);
 else
 printf("  "
"  ");
-- 
2.9.3