| Message ID | 1495007957-5770-1-git-send-email-mchristi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Nick, Do not merge this patch. It will fix the problem, but cause a regression if there was a failure during setup. On 05/17/2017 02:59 AM, Mike Christie wrote: > We currently do > > tcmu_free_device ->tcmu_netlink_event(TCMU_CMD_REMOVED_DEVICE) -> > kfree(tcmu_dev) via rcu call. > > The problem is that the kernel does not wait for userspace to > do close() on the uio device before freeing the tcmu_dev. > We can then hit a race where the kernel frees the tcmu_dev before > userspace does close() and so when close() -> release -> tcmu_release > is done, we try to access a freed tcmu_dev. > > This patch made over the target-pending master branch moves the freeing of > the tcmu_dev to tcmu_release. > > Signed-off-by: Mike Christie <mchristi@redhat.com> > > --- > drivers/target/target_core_user.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c > index 9045837..cda0bd7 100644 > --- a/drivers/target/target_core_user.c > +++ b/drivers/target/target_core_user.c > @@ -1145,14 +1145,26 @@ static int tcmu_open(struct uio_info *info, struct inode *inode) > return 0; > } > > +static void tcmu_dev_call_rcu(struct rcu_head *p) > +{ > + struct se_device *dev = container_of(p, struct se_device, rcu_head); > + struct tcmu_dev *udev = TCMU_DEV(dev); > + > + kfree(udev); > +} > + > static int tcmu_release(struct uio_info *info, struct inode *inode) > { > struct tcmu_dev *udev = container_of(info, struct tcmu_dev, uio_info); > + struct se_device *dev = &udev->se_dev; > > clear_bit(TCMU_DEV_BIT_OPEN, &udev->flags); > > pr_debug("close\n"); > > + kfree(udev->uio_info.name); > + kfree(udev->name); > + call_rcu(&dev->rcu_head, tcmu_dev_call_rcu); > return 0; > } > > @@ -1302,14 +1314,6 @@ static int tcmu_check_and_free_pending_cmd(struct tcmu_cmd *cmd) > return -EINVAL; > } > > -static void tcmu_dev_call_rcu(struct rcu_head *p) > -{ > - struct se_device *dev = container_of(p, struct se_device, rcu_head); > - struct tcmu_dev *udev = TCMU_DEV(dev); > - > - kfree(udev); > -} > - > static bool tcmu_dev_configured(struct tcmu_dev *udev) > { > return udev->uio_info.uio_dev ? true : false; > @@ -1364,10 +1368,9 @@ static void tcmu_free_device(struct se_device *dev) > udev->uio_info.uio_dev->minor); > > uio_unregister_device(&udev->uio_info); > - kfree(udev->uio_info.name); > - kfree(udev->name); > + } else { > + call_rcu(&dev->rcu_head, tcmu_dev_call_rcu); > } > - call_rcu(&dev->rcu_head, tcmu_dev_call_rcu); > } > > enum { > -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index 9045837..cda0bd7 100644 --- a/drivers/target/target_core_user.c +++ b/drivers/target/target_core_user.c @@ -1145,14 +1145,26 @@ static int tcmu_open(struct uio_info *info, struct inode *inode) return 0; } +static void tcmu_dev_call_rcu(struct rcu_head *p) +{ + struct se_device *dev = container_of(p, struct se_device, rcu_head); + struct tcmu_dev *udev = TCMU_DEV(dev); + + kfree(udev); +} + static int tcmu_release(struct uio_info *info, struct inode *inode) { struct tcmu_dev *udev = container_of(info, struct tcmu_dev, uio_info); + struct se_device *dev = &udev->se_dev; clear_bit(TCMU_DEV_BIT_OPEN, &udev->flags); pr_debug("close\n"); + kfree(udev->uio_info.name); + kfree(udev->name); + call_rcu(&dev->rcu_head, tcmu_dev_call_rcu); return 0; } @@ -1302,14 +1314,6 @@ static int tcmu_check_and_free_pending_cmd(struct tcmu_cmd *cmd) return -EINVAL; } -static void tcmu_dev_call_rcu(struct rcu_head *p) -{ - struct se_device *dev = container_of(p, struct se_device, rcu_head); - struct tcmu_dev *udev = TCMU_DEV(dev); - - kfree(udev); -} - static bool tcmu_dev_configured(struct tcmu_dev *udev) { return udev->uio_info.uio_dev ? true : false; @@ -1364,10 +1368,9 @@ static void tcmu_free_device(struct se_device *dev) udev->uio_info.uio_dev->minor); uio_unregister_device(&udev->uio_info); - kfree(udev->uio_info.name); - kfree(udev->name); + } else { + call_rcu(&dev->rcu_head, tcmu_dev_call_rcu); } - call_rcu(&dev->rcu_head, tcmu_dev_call_rcu); } enum {
We currently do tcmu_free_device ->tcmu_netlink_event(TCMU_CMD_REMOVED_DEVICE) -> kfree(tcmu_dev) via rcu call. The problem is that the kernel does not wait for userspace to do close() on the uio device before freeing the tcmu_dev. We can then hit a race where the kernel frees the tcmu_dev before userspace does close() and so when close() -> release -> tcmu_release is done, we try to access a freed tcmu_dev. This patch made over the target-pending master branch moves the freeing of the tcmu_dev to tcmu_release. Signed-off-by: Mike Christie <mchristi@redhat.com> --- drivers/target/target_core_user.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)