Re: [PATCH V14 04/24] mmc: block: Ensure that debugfs files are removed

[Ulf Hansson]

On 21 November 2017 at 14:42, Adrian Hunter  wrote:
> The card is not necessarily being removed, but the debugfs files must be
> removed when the driver is removed, otherwise they will continue to exist
> after unbinding the card from the driver. e.g.
>
>   # echo "mmc1:0001" > /sys/bus/mmc/drivers/mmcblk/unbind
>   # cat /sys/kernel/debug/mmc1/mmc1\:0001/ext_csd
>   [  173.634584] BUG: unable to handle kernel NULL pointer dereference at 
> 0050
>   [  173.643356] IP: mmc_ext_csd_open+0x5e/0x170
>
> A complication is that the debugfs_root may have already been removed, so
> check for that too.
>
> Fixes: 627c3ccfb46a ("mmc: debugfs: Move block debugfs into block module")
> Signed-off-by: Adrian Hunter 

Thanks, applied for fixes and added a stable tag!

Kind regards
Uffe

> ---
>  drivers/mmc/core/block.c   | 44 +---
>  drivers/mmc/core/debugfs.c |  1 +
>  2 files changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 4a319ddbd956..ccfa98af1dd3 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -122,6 +122,10 @@ struct mmc_blk_data {
> struct device_attribute force_ro;
> struct device_attribute power_ro_lock;
> int area_type;
> +
> +   /* debugfs files (only in main mmc_blk_data) */
> +   struct dentry *status_dentry;
> +   struct dentry *ext_csd_dentry;
>  };
>
>  /* Device type for RPMB character devices */
> @@ -2653,7 +2657,7 @@ static int mmc_ext_csd_release(struct inode *inode, 
> struct file *file)
> .llseek = default_llseek,
>  };
>
> -static int mmc_blk_add_debugfs(struct mmc_card *card)
> +static int mmc_blk_add_debugfs(struct mmc_card *card, struct mmc_blk_data 
> *md)
>  {
> struct dentry *root;
>
> @@ -2663,28 +2667,53 @@ static int mmc_blk_add_debugfs(struct mmc_card *card)
> root = card->debugfs_root;
>
> if (mmc_card_mmc(card) || mmc_card_sd(card)) {
> -   if (!debugfs_create_file("status", S_IRUSR, root, card,
> -_dbg_card_status_fops))
> +   md->status_dentry =
> +   debugfs_create_file("status", S_IRUSR, root, card,
> +   _dbg_card_status_fops);
> +   if (!md->status_dentry)
> return -EIO;
> }
>
> if (mmc_card_mmc(card)) {
> -   if (!debugfs_create_file("ext_csd", S_IRUSR, root, card,
> -_dbg_ext_csd_fops))
> +   md->ext_csd_dentry =
> +   debugfs_create_file("ext_csd", S_IRUSR, root, card,
> +   _dbg_ext_csd_fops);
> +   if (!md->ext_csd_dentry)
> return -EIO;
> }
>
> return 0;
>  }
>
> +static void mmc_blk_remove_debugfs(struct mmc_card *card,
> +  struct mmc_blk_data *md)
> +{
> +   if (!card->debugfs_root)
> +   return;
> +
> +   if (!IS_ERR_OR_NULL(md->status_dentry)) {
> +   debugfs_remove(md->status_dentry);
> +   md->status_dentry = NULL;
> +   }
> +
> +   if (!IS_ERR_OR_NULL(md->ext_csd_dentry)) {
> +   debugfs_remove(md->ext_csd_dentry);
> +   md->ext_csd_dentry = NULL;
> +   }
> +}
>
>  #else
>
> -static int mmc_blk_add_debugfs(struct mmc_card *card)
> +static int mmc_blk_add_debugfs(struct mmc_card *card, struct mmc_blk_data 
> *md)
>  {
> return 0;
>  }
>
> +static void mmc_blk_remove_debugfs(struct mmc_card *card,
> +  struct mmc_blk_data *md)
> +{
> +}
> +
>  #endif /* CONFIG_DEBUG_FS */
>
>  static int mmc_blk_probe(struct mmc_card *card)
> @@ -2724,7 +2753,7 @@ static int mmc_blk_probe(struct mmc_card *card)
> }
>
> /* Add two debugfs entries */
> -   mmc_blk_add_debugfs(card);
> +   mmc_blk_add_debugfs(card, md);
>
> pm_runtime_set_autosuspend_delay(>dev, 3000);
> pm_runtime_use_autosuspend(>dev);
> @@ -2750,6 +2779,7 @@ static void mmc_blk_remove(struct mmc_card *card)
>  {
> struct mmc_blk_data *md = dev_get_drvdata(>dev);
>
> +   mmc_blk_remove_debugfs(card, md);
> mmc_blk_remove_parts(card, md);
> pm_runtime_get_sync(>dev);
> mmc_claim_host(card->host);
> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
> index 01e459a34f33..0f4a7d7b2626 100644
> --- a/drivers/mmc/core/debugfs.c
> +++ b/drivers/mmc/core/debugfs.c
> @@ -314,4 +314,5 @@ void mmc_add_card_debugfs(struct mmc_card *card)
>  void mmc_remove_card_debugfs(struct mmc_card *card)
>  {
> debugfs_remove_recursive(card->debugfs_root);
> +   card->debugfs_root = NULL;
>  }
> --
> 1.9.1
>

Re: [PATCH V14 04/24] mmc: block: Ensure that debugfs files are removed

[Linus Walleij]

On Tue, Nov 21, 2017 at 2:42 PM, Adrian Hunter  wrote:

> The card is not necessarily being removed, but the debugfs files must be
> removed when the driver is removed, otherwise they will continue to exist
> after unbinding the card from the driver. e.g.
>
>   # echo "mmc1:0001" > /sys/bus/mmc/drivers/mmcblk/unbind
>   # cat /sys/kernel/debug/mmc1/mmc1\:0001/ext_csd
>   [  173.634584] BUG: unable to handle kernel NULL pointer dereference at 
> 0050
>   [  173.643356] IP: mmc_ext_csd_open+0x5e/0x170
>
> A complication is that the debugfs_root may have already been removed, so
> check for that too.
>
> Fixes: 627c3ccfb46a ("mmc: debugfs: Move block debugfs into block module")
> Signed-off-by: Adrian Hunter 

Reviewed-by: Linus Walleij 

I was assuming debugfs would always be removed from debugfs.c
using debugfs_remove_recursive(card->debugfs_root) but that
doesn't work for this case where we bin/unbind the block layer
interactively, sorry for missing it :(

Ulf: I think this can go in as an *early* fix as well, say after -rc1.

Yours,
Linus Walleij

[PATCH V14 04/24] mmc: block: Ensure that debugfs files are removed

[Adrian Hunter]

The card is not necessarily being removed, but the debugfs files must be
removed when the driver is removed, otherwise they will continue to exist
after unbinding the card from the driver. e.g.

  # echo "mmc1:0001" > /sys/bus/mmc/drivers/mmcblk/unbind
  # cat /sys/kernel/debug/mmc1/mmc1\:0001/ext_csd
  [  173.634584] BUG: unable to handle kernel NULL pointer dereference at 
0050
  [  173.643356] IP: mmc_ext_csd_open+0x5e/0x170

A complication is that the debugfs_root may have already been removed, so
check for that too.

Fixes: 627c3ccfb46a ("mmc: debugfs: Move block debugfs into block module")
Signed-off-by: Adrian Hunter 
---
 drivers/mmc/core/block.c   | 44 +---
 drivers/mmc/core/debugfs.c |  1 +
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 4a319ddbd956..ccfa98af1dd3 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -122,6 +122,10 @@ struct mmc_blk_data {
struct device_attribute force_ro;
struct device_attribute power_ro_lock;
int area_type;
+
+   /* debugfs files (only in main mmc_blk_data) */
+   struct dentry *status_dentry;
+   struct dentry *ext_csd_dentry;
 };
 
 /* Device type for RPMB character devices */
@@ -2653,7 +2657,7 @@ static int mmc_ext_csd_release(struct inode *inode, 
struct file *file)
.llseek = default_llseek,
 };
 
-static int mmc_blk_add_debugfs(struct mmc_card *card)
+static int mmc_blk_add_debugfs(struct mmc_card *card, struct mmc_blk_data *md)
 {
struct dentry *root;
 
@@ -2663,28 +2667,53 @@ static int mmc_blk_add_debugfs(struct mmc_card *card)
root = card->debugfs_root;
 
if (mmc_card_mmc(card) || mmc_card_sd(card)) {
-   if (!debugfs_create_file("status", S_IRUSR, root, card,
-_dbg_card_status_fops))
+   md->status_dentry =
+   debugfs_create_file("status", S_IRUSR, root, card,
+   _dbg_card_status_fops);
+   if (!md->status_dentry)
return -EIO;
}
 
if (mmc_card_mmc(card)) {
-   if (!debugfs_create_file("ext_csd", S_IRUSR, root, card,
-_dbg_ext_csd_fops))
+   md->ext_csd_dentry =
+   debugfs_create_file("ext_csd", S_IRUSR, root, card,
+   _dbg_ext_csd_fops);
+   if (!md->ext_csd_dentry)
return -EIO;
}
 
return 0;
 }
 
+static void mmc_blk_remove_debugfs(struct mmc_card *card,
+  struct mmc_blk_data *md)
+{
+   if (!card->debugfs_root)
+   return;
+
+   if (!IS_ERR_OR_NULL(md->status_dentry)) {
+   debugfs_remove(md->status_dentry);
+   md->status_dentry = NULL;
+   }
+
+   if (!IS_ERR_OR_NULL(md->ext_csd_dentry)) {
+   debugfs_remove(md->ext_csd_dentry);
+   md->ext_csd_dentry = NULL;
+   }
+}
 
 #else
 
-static int mmc_blk_add_debugfs(struct mmc_card *card)
+static int mmc_blk_add_debugfs(struct mmc_card *card, struct mmc_blk_data *md)
 {
return 0;
 }
 
+static void mmc_blk_remove_debugfs(struct mmc_card *card,
+  struct mmc_blk_data *md)
+{
+}
+
 #endif /* CONFIG_DEBUG_FS */
 
 static int mmc_blk_probe(struct mmc_card *card)
@@ -2724,7 +2753,7 @@ static int mmc_blk_probe(struct mmc_card *card)
}
 
/* Add two debugfs entries */
-   mmc_blk_add_debugfs(card);
+   mmc_blk_add_debugfs(card, md);
 
pm_runtime_set_autosuspend_delay(>dev, 3000);
pm_runtime_use_autosuspend(>dev);
@@ -2750,6 +2779,7 @@ static void mmc_blk_remove(struct mmc_card *card)
 {
struct mmc_blk_data *md = dev_get_drvdata(>dev);
 
+   mmc_blk_remove_debugfs(card, md);
mmc_blk_remove_parts(card, md);
pm_runtime_get_sync(>dev);
mmc_claim_host(card->host);
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 01e459a34f33..0f4a7d7b2626 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -314,4 +314,5 @@ void mmc_add_card_debugfs(struct mmc_card *card)
 void mmc_remove_card_debugfs(struct mmc_card *card)
 {
debugfs_remove_recursive(card->debugfs_root);
+   card->debugfs_root = NULL;
 }
-- 
1.9.1