Re: [PATCH 1/2 v6] mmc: block: Convert RPMB to a character device

[Jisheng Zhang]

On Wed, 27 Sep 2017 21:45:58 +0200 Linus Walleij wrote:

> On Wed, Sep 27, 2017 at 1:35 PM, Adrian Hunter  
> wrote:
> >
> > BUG when removing, fixed by reverting this patch.
> >
> > [  346.548512] mmc1: card 0001 removed
> > [  346.552782] BUG: unable to handle kernel NULL pointer dereference at 
> > 0070  
> 
> How did you achieve this? I need to reproduce it.
> 
> RPMB is only available on eMMC cards and I don't have a removable eMMC
> card since by definition they are soldered on.

Maybe unbind the sdhci host.

Re: [PATCH 1/2 v6] mmc: block: Convert RPMB to a character device

[Shawn Lin]

On 2017/9/28 3:45, Linus Walleij wrote:

On Wed, Sep 27, 2017 at 1:35 PM, Adrian Hunter  wrote:


BUG when removing, fixed by reverting this patch.

[  346.548512] mmc1: card 0001 removed
[  346.552782] BUG: unable to handle kernel NULL pointer dereference at 
0070


How did you achieve this? I need to reproduce it.

RPMB is only available on eMMC cards and I don't have a removable eMMC
card since by definition they are soldered on.

Does it happen on an ordinary MMC card without RPMB?

I'm sorry if I'm ignorant of something very obvious here...

I will read the code and see if I'm missing something obvious.


Haven't closely look at this patch, but btw, don't you need
put_device(>dev) for the error path of  mmc_blk_alloc_rpmb_part
and  also do that for mmc_blk_remove_rpmb_part?

And don't you need to check rpmb->md->usage when doing
mmc_blk_remove_rpmb_part ?



Yours,
Linus Walleij

Re: [PATCH 1/2 v6] mmc: block: Convert RPMB to a character device

[Linus Walleij]

On Wed, Sep 27, 2017 at 1:35 PM, Adrian Hunter  wrote:
>
> BUG when removing, fixed by reverting this patch.
>
> [  346.548512] mmc1: card 0001 removed
> [  346.552782] BUG: unable to handle kernel NULL pointer dereference at 
> 0070

How did you achieve this? I need to reproduce it.

RPMB is only available on eMMC cards and I don't have a removable eMMC
card since by definition they are soldered on.

Does it happen on an ordinary MMC card without RPMB?

I'm sorry if I'm ignorant of something very obvious here...

I will read the code and see if I'm missing something obvious.

Yours,
Linus Walleij

Re: [PATCH 1/2 v6] mmc: block: Convert RPMB to a character device

[Adrian Hunter]

BUG when removing, fixed by reverting this patch.

[  346.548512] mmc1: card 0001 removed
[  346.552782] BUG: unable to handle kernel NULL pointer dereference at 
0070
[  346.561539] IP: kernfs_find_ns+0xe/0xc0
[  346.565822] PGD 179dc5067 P4D 179dc5067 PUD 171106067 PMD 0 
[  346.572152] Oops:  [#1] SMP
[  346.575657] Modules linked in:
[  346.579069] CPU: 0 PID: 1393 Comm: sh Not tainted 
4.14.0-rc1-00034-g5aa83fb178e3 #1528
[  346.587914] task: 899d3a3327c0 task.stack: 9e6600888000
[  346.594530] RIP: 0010:kernfs_find_ns+0xe/0xc0
[  346.599395] RSP: :9e660088ba28 EFLAGS: 00010246
[  346.605229] RAX:  RBX:  RCX: 
[  346.613199] RDX:  RSI: 90cab2c1 RDI: 
[  346.621168] RBP: 9e660088ba48 R08:  R09: 00018040003d
[  346.629139] R10: 9e660088ba68 R11: 899d3b080e00 R12: 90cab2c1
[  346.637107] R13:  R14: 899d310b04a8 R15: 899d39cdcd70
[  346.645079] FS:  () GS:899d3fc0(0063) 
knlGS:f7f2d690
[  346.654118] CS:  0010 DS: 002b ES: 002b CR0: 80050033
[  346.660536] CR2: 0070 CR3: 000171173000 CR4: 003406f0
[  346.668504] Call Trace:
[  346.671237]  kernfs_find_and_get_ns+0x2c/0x50
[  346.676106]  sysfs_unmerge_group+0x18/0x60
[  346.680683]  dpm_sysfs_remove+0x1d/0x60
[  346.684969]  device_del+0xef/0x2e0
[  346.688765]  ? cdev_del+0x22/0x30
[  346.692470]  mmc_blk_remove_parts.isra.28+0x6c/0xf0
[  346.697919]  mmc_blk_remove+0x27/0x190
[  346.702106]  mmc_bus_remove+0x18/0x20
[  346.706197]  device_release_driver_internal+0x142/0x200
[  346.712036]  device_release_driver+0xd/0x10
[  346.716709]  bus_remove_device+0xdb/0x120
[  346.721188]  device_del+0x1c3/0x2e0
[  346.725083]  mmc_remove_card+0x5b/0xc0
[  346.729270]  mmc_remove+0x14/0x30
[  346.732974]  mmc_stop_host+0xef/0x1a0
[  346.737065]  mmc_remove_host+0x15/0x40

Re: [PATCH 1/2 v6] mmc: block: Convert RPMB to a character device

[Ulf Hansson]

On 20 September 2017 at 10:02, Linus Walleij  wrote:
> The RPMB partition on the eMMC devices is a special area used
> for storing cryptographically safe information signed by a
> special secret key. To write and read records from this special
> area, authentication is needed.
>
> The RPMB area is *only* and *exclusively* accessed using
> ioctl():s from userspace. It is not really a block device,
> as blocks cannot be read or written from the device, also
> the signed chunks that can be stored on the RPMB are actually
> 256 bytes, not 512 making a block device a real bad fit.
>
> Currently the RPMB partition spawns a separate block device
> named /dev/mmcblkNrpmb for each device with an RPMB partition,
> including the creation of a block queue with its own kernel
> thread and all overhead associated with this. On the Ux500
> HREFv60 platform, for example, the two eMMCs means that two
> block queues with separate threads are created for no use
> whatsoever.
>
> I have concluded that this block device design for RPMB is
> actually pretty wrong. The RPMB area should have been designed
> to be accessed from /dev/mmcblkN directly, using ioctl()s on
> the main block device. It is however way too late to change
> that, since userspace expects to open an RPMB device in
> /dev/mmcblkNrpmb and we cannot break userspace.
>
> This patch tries to amend the situation using the following
> strategy:
>
> - Stop creating a block device for the RPMB partition/area
>
> - Instead create a custom, dynamic character device with
>   the same name.
>
> - Make this new character device support exactly the same
>   set of ioctl()s as the old block device.
>
> - Wrap the requests back to the same ioctl() handlers, but
>   issue them on the block queue of the main partition/area,
>   i.e. /dev/mmcblkN
>
> We need to create a special "rpmb" bus type in order to get
> udev and/or busybox hot/coldplug to instantiate the device
> node properly.
>
> Before the patch, this appears in 'ps aux':
>
> 101 root   0:00 [mmcqd/2rpmb]
> 123 root   0:00 [mmcqd/3rpmb]
>
> After applying the patch these surplus block queue threads
> are gone, but RPMB is as usable as ever using the userspace
> MMC tools, such as 'mmc rpmb read-counter'.
>
> We get instead those dynamice devices in /dev:
>
> brw-rw1 root root  179,   0 Jan  1  2000 mmcblk0
> brw-rw1 root root  179,   1 Jan  1  2000 mmcblk0p1
> brw-rw1 root root  179,   2 Jan  1  2000 mmcblk0p2
> brw-rw1 root root  179,   5 Jan  1  2000 mmcblk0p5
> brw-rw1 root root  179,   8 Jan  1  2000 mmcblk2
> brw-rw1 root root  179,  16 Jan  1  2000 mmcblk2boot0
> brw-rw1 root root  179,  24 Jan  1  2000 mmcblk2boot1
> crw-rw1 root root  248,   0 Jan  1  2000 mmcblk2rpmb
> brw-rw1 root root  179,  32 Jan  1  2000 mmcblk3
> brw-rw1 root root  179,  40 Jan  1  2000 mmcblk3boot0
> brw-rw1 root root  179,  48 Jan  1  2000 mmcblk3boot1
> brw-rw1 root root  179,  33 Jan  1  2000 mmcblk3p1
> crw-rw1 root root  248,   1 Jan  1  2000 mmcblk3rpmb
>
> Notice the (248,0) and (248,1) character devices for RPMB.
>
> Cc: Tomas Winkler 
> Signed-off-by: Linus Walleij 

Thanks, applied for next!

Kind regards
Uffe

> ---
> ChangeLog v5->v6:
> - Prefix the bus name with mmc_ so it becomes "mmc_rpmb"
> - Prefix the RPMB-specific symbols with mmc_*
> - Use the ternary operator ( = rpmb ? A : B ) for assigning IOCTL
>   enums
> ChangeLog v1 (RFC) -> v5:
> - Rebase.
> - Drop discussion comments, let's go for this unless someone
>   has a better idea.
> - Rename rpmb_devt and rpmb_bus_type to mmc_rpmb_devt and
>   mmc_rpmb_bus_type as requested by Tomas.
> - Handle multiple RPMB partitions as requested by Tomas.
> - Renumber v5 to keep together with the rest of the series.
> ---
>  drivers/mmc/core/block.c | 283 
> +++
>  drivers/mmc/core/queue.h |   2 +
>  2 files changed, 263 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 29fc1e662891..6421d06b66bb 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -86,6 +87,7 @@ static int max_devices;
>  #define MAX_DEVICES 256
>
>  static DEFINE_IDA(mmc_blk_ida);
> +static DEFINE_IDA(mmc_rpmb_ida);
>
>  /*
>   * There is one mmc_blk_data per slot.
> @@ -96,6 +98,7 @@ struct mmc_blk_data {
> struct gendisk  *disk;
> struct mmc_queue queue;
> struct list_head part;
> +   struct list_head rpmbs;
>
> unsigned intflags;
>  #define MMC_BLK_CMD23  (1 << 0)/* Can do SET_BLOCK_COUNT for 
> multiblock */
> @@ -121,6 +124,32 @@ struct 

[PATCH 1/2 v6] mmc: block: Convert RPMB to a character device

[Linus Walleij]

The RPMB partition on the eMMC devices is a special area used
for storing cryptographically safe information signed by a
special secret key. To write and read records from this special
area, authentication is needed.

The RPMB area is *only* and *exclusively* accessed using
ioctl():s from userspace. It is not really a block device,
as blocks cannot be read or written from the device, also
the signed chunks that can be stored on the RPMB are actually
256 bytes, not 512 making a block device a real bad fit.

Currently the RPMB partition spawns a separate block device
named /dev/mmcblkNrpmb for each device with an RPMB partition,
including the creation of a block queue with its own kernel
thread and all overhead associated with this. On the Ux500
HREFv60 platform, for example, the two eMMCs means that two
block queues with separate threads are created for no use
whatsoever.

I have concluded that this block device design for RPMB is
actually pretty wrong. The RPMB area should have been designed
to be accessed from /dev/mmcblkN directly, using ioctl()s on
the main block device. It is however way too late to change
that, since userspace expects to open an RPMB device in
/dev/mmcblkNrpmb and we cannot break userspace.

This patch tries to amend the situation using the following
strategy:

- Stop creating a block device for the RPMB partition/area

- Instead create a custom, dynamic character device with
  the same name.

- Make this new character device support exactly the same
  set of ioctl()s as the old block device.

- Wrap the requests back to the same ioctl() handlers, but
  issue them on the block queue of the main partition/area,
  i.e. /dev/mmcblkN

We need to create a special "rpmb" bus type in order to get
udev and/or busybox hot/coldplug to instantiate the device
node properly.

Before the patch, this appears in 'ps aux':

101 root   0:00 [mmcqd/2rpmb]
123 root   0:00 [mmcqd/3rpmb]

After applying the patch these surplus block queue threads
are gone, but RPMB is as usable as ever using the userspace
MMC tools, such as 'mmc rpmb read-counter'.

We get instead those dynamice devices in /dev:

brw-rw1 root root  179,   0 Jan  1  2000 mmcblk0
brw-rw1 root root  179,   1 Jan  1  2000 mmcblk0p1
brw-rw1 root root  179,   2 Jan  1  2000 mmcblk0p2
brw-rw1 root root  179,   5 Jan  1  2000 mmcblk0p5
brw-rw1 root root  179,   8 Jan  1  2000 mmcblk2
brw-rw1 root root  179,  16 Jan  1  2000 mmcblk2boot0
brw-rw1 root root  179,  24 Jan  1  2000 mmcblk2boot1
crw-rw1 root root  248,   0 Jan  1  2000 mmcblk2rpmb
brw-rw1 root root  179,  32 Jan  1  2000 mmcblk3
brw-rw1 root root  179,  40 Jan  1  2000 mmcblk3boot0
brw-rw1 root root  179,  48 Jan  1  2000 mmcblk3boot1
brw-rw1 root root  179,  33 Jan  1  2000 mmcblk3p1
crw-rw1 root root  248,   1 Jan  1  2000 mmcblk3rpmb

Notice the (248,0) and (248,1) character devices for RPMB.

Cc: Tomas Winkler 
Signed-off-by: Linus Walleij 
---
ChangeLog v5->v6:
- Prefix the bus name with mmc_ so it becomes "mmc_rpmb"
- Prefix the RPMB-specific symbols with mmc_*
- Use the ternary operator ( = rpmb ? A : B ) for assigning IOCTL
  enums
ChangeLog v1 (RFC) -> v5:
- Rebase.
- Drop discussion comments, let's go for this unless someone
  has a better idea.
- Rename rpmb_devt and rpmb_bus_type to mmc_rpmb_devt and
  mmc_rpmb_bus_type as requested by Tomas.
- Handle multiple RPMB partitions as requested by Tomas.
- Renumber v5 to keep together with the rest of the series.
---
 drivers/mmc/core/block.c | 283 +++
 drivers/mmc/core/queue.h |   2 +
 2 files changed, 263 insertions(+), 22 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 29fc1e662891..6421d06b66bb 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -86,6 +87,7 @@ static int max_devices;
 #define MAX_DEVICES 256
 
 static DEFINE_IDA(mmc_blk_ida);
+static DEFINE_IDA(mmc_rpmb_ida);
 
 /*
  * There is one mmc_blk_data per slot.
@@ -96,6 +98,7 @@ struct mmc_blk_data {
struct gendisk  *disk;
struct mmc_queue queue;
struct list_head part;
+   struct list_head rpmbs;
 
unsigned intflags;
 #define MMC_BLK_CMD23  (1 << 0)/* Can do SET_BLOCK_COUNT for 
multiblock */
@@ -121,6 +124,32 @@ struct mmc_blk_data {
int area_type;
 };
 
+/* Device type for RPMB character devices */
+static dev_t mmc_rpmb_devt;
+
+/* Bus type for RPMB character devices */
+static struct bus_type mmc_rpmb_bus_type = {
+   .name = "mmc_rpmb",
+};
+
+/**
+ * struct mmc_rpmb_data - special RPMB device type for these areas
+ * @dev: the device for the