[PATCH] mmc: block: Fix bug when removing RPMB chardev

[PATCH] mmc: block: Fix bug when removing RPMB chardev

[Linus Walleij]

I forgot to account for the fact that the device core holds a
reference to a device added with device_initialize() that need
to be released with a corresponding put_device() to reach a 0
refcount at the end of the lifecycle.

This led to a NULL pointer reference when freeing the device
when e.g. unbidning the host device in sysfs.

Fix this and use the device .release() callback to free the
IDA and free:ing the memory used by the RPMB device.

Before this patch:

/sys/bus/amba/drivers/mmci-pl18x$ echo 80114000.sdi4_per2 > unbind
[   29.797332] mmc3: card 0001 removed
[   29.810791] Unable to handle kernel NULL pointer dereference at
               virtual address 00000050
[   29.818878] pgd = de70c000
[   29.821624] [00000050] *pgd=1e70a831, *pte=00000000, *ppte=00000000
[   29.827911] Internal error: Oops: 17 [#1] PREEMPT SMP ARM
[   29.833282] Modules linked in:
[   29.836334] CPU: 1 PID: 154 Comm: sh Not tainted
               4.14.0-rc3-00039-g83318e309566-dirty #736
[   29.844604] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[   29.851562] task: de572700 task.stack: de742000
[   29.856079] PC is at kernfs_find_ns+0x8/0x100
[   29.860443] LR is at kernfs_find_and_get_ns+0x30/0x48

After this patch:

/sys/bus/amba/drivers/mmci-pl18x$ echo 80005000.sdi4_per2 > unbind
[   20.623382] mmc3: card 0001 removed
[   20.627288] mmc_rpmb mmcblk3rpmb: removed

Fixes: 0ab0c7c0e65a ("mmc: block: Convert RPMB to a character device")
Reported-by: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index c29dbcec7c61..2033a71bacc3 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2329,6 +2329,14 @@ static const struct file_operations mmc_rpmb_fileops = {
 #endif
 };
 
+static void mmc_blk_rpmb_device_release(struct device *dev)
+{
+	struct mmc_rpmb_data *rpmb = dev_get_drvdata(dev);
+
+	dev_info(dev, "removed\n");
+	ida_simple_remove(&mmc_rpmb_ida, rpmb->id);
+	kfree(rpmb);
+}
 
 static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
 				   struct mmc_blk_data *md,
@@ -2371,6 +2379,8 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
 		goto out_remove_ida;
 	}
 
+	/* From this point, the .release() function cleans up the device */
+	rpmb->dev.release = mmc_blk_rpmb_device_release;
 	list_add(&rpmb->node, &md->rpmbs);
 
 	string_get_size((u64)size, 512, STRING_UNITS_2,
@@ -2384,17 +2394,22 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
 	return 0;
 
 out_remove_ida:
+	put_device(&rpmb->dev);
 	ida_simple_remove(&mmc_rpmb_ida, rpmb->id);
 	kfree(rpmb);
 	return ret;
 }
 
 static void mmc_blk_remove_rpmb_part(struct mmc_rpmb_data *rpmb)
+
 {
 	cdev_device_del(&rpmb->chrdev, &rpmb->dev);
-	device_del(&rpmb->dev);
-	ida_simple_remove(&mmc_rpmb_ida, rpmb->id);
-	kfree(rpmb);
+	/*
+	 * Unless something is holding a reference to the device, this
+	 * drops the last reference and triggers the device to cleanup
+	 * and calls the .remove() callback.
+	 */
+	put_device(&rpmb->dev);
 }
 
 /* MMC Physical partitions consist of two boot partitions and
-- 
2.13.5

Re: [PATCH] mmc: block: Fix bug when removing RPMB chardev

[Adrian Hunter]

On 02/10/17 14:59, Linus Walleij wrote:
> I forgot to account for the fact that the device core holds a
> reference to a device added with device_initialize() that need
> to be released with a corresponding put_device() to reach a 0
> refcount at the end of the lifecycle.
> 
> This led to a NULL pointer reference when freeing the device
> when e.g. unbidning the host device in sysfs.
> 
> Fix this and use the device .release() callback to free the
> IDA and free:ing the memory used by the RPMB device.
> 
> Before this patch:
> 
> /sys/bus/amba/drivers/mmci-pl18x$ echo 80114000.sdi4_per2 > unbind
> [   29.797332] mmc3: card 0001 removed
> [   29.810791] Unable to handle kernel NULL pointer dereference at
>                virtual address 00000050
> [   29.818878] pgd = de70c000
> [   29.821624] [00000050] *pgd=1e70a831, *pte=00000000, *ppte=00000000
> [   29.827911] Internal error: Oops: 17 [#1] PREEMPT SMP ARM
> [   29.833282] Modules linked in:
> [   29.836334] CPU: 1 PID: 154 Comm: sh Not tainted
>                4.14.0-rc3-00039-g83318e309566-dirty #736
> [   29.844604] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
> [   29.851562] task: de572700 task.stack: de742000
> [   29.856079] PC is at kernfs_find_ns+0x8/0x100
> [   29.860443] LR is at kernfs_find_and_get_ns+0x30/0x48
> 
> After this patch:
> 
> /sys/bus/amba/drivers/mmci-pl18x$ echo 80005000.sdi4_per2 > unbind
> [   20.623382] mmc3: card 0001 removed
> [   20.627288] mmc_rpmb mmcblk3rpmb: removed
> 
> Fixes: 0ab0c7c0e65a ("mmc: block: Convert RPMB to a character device")
> Reported-by: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/core/block.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index c29dbcec7c61..2033a71bacc3 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -2329,6 +2329,14 @@ static const struct file_operations mmc_rpmb_fileops = {
>  #endif
>  };
>  
> +static void mmc_blk_rpmb_device_release(struct device *dev)
> +{
> +	struct mmc_rpmb_data *rpmb = dev_get_drvdata(dev);
> +
> +	dev_info(dev, "removed\n");

Do we really need this message?

> +	ida_simple_remove(&mmc_rpmb_ida, rpmb->id);
> +	kfree(rpmb);
> +}
>  
>  static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
>  				   struct mmc_blk_data *md,
> @@ -2371,6 +2379,8 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
>  		goto out_remove_ida;
>  	}
>  
> +	/* From this point, the .release() function cleans up the device */
> +	rpmb->dev.release = mmc_blk_rpmb_device_release;

This should be before device_initialize(&rpmb->dev) then the error path
becomes just put_device(&rpmb->dev).

But further up is:
	rpmb = kzalloc(sizeof(*rpmb), GFP_KERNEL);
	if (!rpmb)
		return -ENOMEM;
where it looks like you are leaking the ida.

>  	list_add(&rpmb->node, &md->rpmbs);
>  
>  	string_get_size((u64)size, 512, STRING_UNITS_2,
> @@ -2384,17 +2394,22 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
>  	return 0;
>  
>  out_remove_ida:
> +	put_device(&rpmb->dev);
>  	ida_simple_remove(&mmc_rpmb_ida, rpmb->id);
>  	kfree(rpmb);
>  	return ret;
>  }
>  
>  static void mmc_blk_remove_rpmb_part(struct mmc_rpmb_data *rpmb)
> +
>  {
>  	cdev_device_del(&rpmb->chrdev, &rpmb->dev);
> -	device_del(&rpmb->dev);
> -	ida_simple_remove(&mmc_rpmb_ida, rpmb->id);
> -	kfree(rpmb);
> +	/*
> +	 * Unless something is holding a reference to the device, this
> +	 * drops the last reference and triggers the device to cleanup
> +	 * and calls the .remove() callback.
> +	 */

This comment seems redundant since it is just describing how reference
counting works.

> +	put_device(&rpmb->dev);
>  }
>  
>  /* MMC Physical partitions consist of two boot partitions and
> 

Also I noticed this function:

static int mmc_rpmb_chrdev_release(struct inode *inode, struct file *filp)
{
	struct mmc_rpmb_data *rpmb = container_of(inode->i_cdev,
						  struct mmc_rpmb_data, chrdev);

	put_device(&rpmb->dev);
	mutex_lock(&open_lock);
	rpmb->md->usage--;
	mutex_unlock(&open_lock);

	return 0;
}

What is going on with 'usage'?  It looks weird.
What happens if you do this:
	open the rpmb device
	unbind the host controller
	try to use an ioctl
	close the rpmb device

Re: [PATCH] mmc: block: Fix bug when removing RPMB chardev

[Linus Walleij]

On Tue, Oct 3, 2017 at 10:32 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 02/10/17 14:59, Linus Walleij wrote:

>> +static void mmc_blk_rpmb_device_release(struct device *dev)
>> +{
>> +     struct mmc_rpmb_data *rpmb = dev_get_drvdata(dev);
>> +
>> +     dev_info(dev, "removed\n");
>
> Do we really need this message?

Nah. More for symmetry since we print when we add partitions
we also print when we remove them so it seemed consistent to
me.

>> +     /* From this point, the .release() function cleans up the device */
>> +     rpmb->dev.release = mmc_blk_rpmb_device_release;
>
> This should be before device_initialize(&rpmb->dev) then the error path
> becomes just put_device(&rpmb->dev).

The way I dealt with it in the GPIO subsystem was to not assign
this pointer until later, and manage resources in the initilializer
function (such as the error path) until that point.

But I rewrote it like you say.

> But further up is:
>         rpmb = kzalloc(sizeof(*rpmb), GFP_KERNEL);
>         if (!rpmb)
>                 return -ENOMEM;
> where it looks like you are leaking the ida.

OMG you're right. Fixed it for v2.

>> -     device_del(&rpmb->dev);
>> -     ida_simple_remove(&mmc_rpmb_ida, rpmb->id);
>> -     kfree(rpmb);
>> +     /*
>> +      * Unless something is holding a reference to the device, this
>> +      * drops the last reference and triggers the device to cleanup
>> +      * and calls the .remove() callback.
>> +      */
>
> This comment seems redundant since it is just describing how reference
> counting works.

OK I cut the crap. :)

> Also I noticed this function:
>
> static int mmc_rpmb_chrdev_release(struct inode *inode, struct file *filp)
> {
>         struct mmc_rpmb_data *rpmb = container_of(inode->i_cdev,
>                                                   struct mmc_rpmb_data, chrdev);
>
>         put_device(&rpmb->dev);
>         mutex_lock(&open_lock);
>         rpmb->md->usage--;
>         mutex_unlock(&open_lock);
>
>         return 0;
> }
>
> What is going on with 'usage'?  It looks weird.

It is the same reference counting as was present before the patch
converting the block device to a character device. Not my invention.
It's been there since the conception of the MMC stack.

While the get_device()/put_device() is reference counting the
RPMB device per se, this is refcounting the block device that
is backing the RPMB char device, so that the block device
cannot be removed if the character device is using it for
RPMB access.

> What happens if you do this:
>         open the rpmb device
>         unbind the host controller
>         try to use an ioctl
>         close the rpmb device

It's analogous to the similar usecase I had in GPIO:

What if you're holding (in userspace) a reference to a resource and
the hardware backing the resource goes away?

In GPIO I chose to "numb" the device so that any further ioctl()s
would just be silently ignored but for RPMB I guess we should
simply start returning errors.

Since I'm still supporting the old refcounting with md->usage
as described above, it should behave the same as the old
codebase, which might not be very good behaviour but atleast
it is not a regression.

Posting the v2 soon.

Yours,
Linus Walleij

Re: [PATCH] mmc: block: Fix bug when removing RPMB chardev

[Adrian Hunter]

On 03/10/17 13:00, Linus Walleij wrote:
> On Tue, Oct 3, 2017 at 10:32 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Also I noticed this function:
>>
>> static int mmc_rpmb_chrdev_release(struct inode *inode, struct file *filp)
>> {
>>         struct mmc_rpmb_data *rpmb = container_of(inode->i_cdev,
>>                                                   struct mmc_rpmb_data, chrdev);
>>
>>         put_device(&rpmb->dev);
>>         mutex_lock(&open_lock);
>>         rpmb->md->usage--;
>>         mutex_unlock(&open_lock);
>>
>>         return 0;
>> }
>>
>> What is going on with 'usage'?  It looks weird.
> 
> It is the same reference counting as was present before the patch
> converting the block device to a character device. Not my invention.
> It's been there since the conception of the MMC stack.
> 
> While the get_device()/put_device() is reference counting the
> RPMB device per se, this is refcounting the block device that
> is backing the RPMB char device, so that the block device
> cannot be removed if the character device is using it for
> RPMB access.
> 
>> What happens if you do this:
>>         open the rpmb device
>>         unbind the host controller
>>         try to use an ioctl
>>         close the rpmb device
> 
> It's analogous to the similar usecase I had in GPIO:
> 
> What if you're holding (in userspace) a reference to a resource and
> the hardware backing the resource goes away?
> 
> In GPIO I chose to "numb" the device so that any further ioctl()s
> would just be silently ignored but for RPMB I guess we should
> simply start returning errors.
> 
> Since I'm still supporting the old refcounting with md->usage
> as described above, it should behave the same as the old
> codebase, which might not be very good behaviour but atleast
> it is not a regression.

How do you know you are not dropping the last reference when you do
md->usage-- ?

Re: [PATCH] mmc: block: Fix bug when removing RPMB chardev

[Linus Walleij]

On Tue, Oct 3, 2017 at 1:07 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 03/10/17 13:00, Linus Walleij wrote:

>> Since I'm still supporting the old refcounting with md->usage
>> as described above, it should behave the same as the old
>> codebase, which might not be very good behaviour but atleast
>> it is not a regression.
>
> How do you know you are not dropping the last reference when you do
> md->usage-- ?

Ah dammit what was I thinking... OK converting to mmc_blk_get()
+mmc_blk_put() that should do it.

Sorry for silly mistakes. :/

Yours,
Linus Walleij

Re: [PATCH] mmc: block: Fix bug when removing RPMB chardev

[Linus Walleij]

On Tue, Oct 3, 2017 at 11:57 PM, Avri Altman <Avri.Altman@wdc.com> wrote:

> Multiple simultaneous accesses is forbidden.
> So maybe direct recounting is better for controlling it?

That is already handled by patches:

commit 614f0388f580c436d2cf6dc0855de91d13ddc23d
"mmc: block: move single ioctl() commands to block requests"
commit 3ecd8cf23f88d5df1c545a5c04217987abb28575
"mmc: block: move multi-ioctl() to use block layer"

Since all ioctl()s are not sent in through the block queue
they get serialized by the very nature of how block requests
work, i.e. one at a time. I have even tried to send RPMB
commands from two consoles while a huge dd work was already
working in the background. No problems with that.

Yours,
Linus Walleij

Re: [PATCH] mmc: block: Fix bug when removing RPMB chardev

[Linus Walleij]

On Wed, Oct 4, 2017 at 8:14 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Oct 3, 2017 at 11:57 PM, Avri Altman <Avri.Altman@wdc.com> wrote:
>
>> Multiple simultaneous accesses is forbidden.
>> So maybe direct recounting is better for controlling it?
>
> That is already handled by patches:
>
> commit 614f0388f580c436d2cf6dc0855de91d13ddc23d
> "mmc: block: move single ioctl() commands to block requests"
> commit 3ecd8cf23f88d5df1c545a5c04217987abb28575
> "mmc: block: move multi-ioctl() to use block layer"
>
> Since all ioctl()s are not sent in through the block queue

Since all ioctl()s are NOW sent in through the block queue

> they get serialized by the very nature of how block requests
> work, i.e. one at a time. I have even tried to send RPMB
> commands from two consoles while a huge dd work was already
> working in the background. No problems with that.

Sorry for spellling mistakes.

Yours,
Linus Walleij