[stable/4.14.y PATCH 0/3] mmc: Fix a potential resource leak when shutting down request queue.

[stable/4.14.y PATCH 0/3] mmc: Fix a potential resource leak when shutting down request queue.

[Raul E Rangel]

I think we should cherry-pick 41e3efd07d5a02c80f503e29d755aa1bbb4245de
https://lore.kernel.org/patchwork/patch/856512/ into 4.14. It fixes a
potential resource leak when shutting down the request queue.

Once this patch is applied, there is a potential for a null pointer dereference.
That's what the second patch fixes.

The third patch is just an optimization to stop processing earlier.

See https://patchwork.kernel.org/patch/10925469/ for the initial motivation.

This commit applies to v4.14.116. It is already included in 4.19. 4.19 doesn't
suffer from the null pointer dereference because later commits migrate the mmc
stack to blk-mq.

I tested this patch set by randomly connecting/disconnecting the SD
card. I got over 189650 itarations without a problem.

Thanks,
Raul


Adrian Hunter (1):
  mmc: block: Simplify cleaning up the queue

Raul E Rangel (2):
  mmc: Fix null pointer dereference in mmc_init_request
  mmc: Kill the request if the queuedata has been removed

 drivers/mmc/core/block.c | 17 ++++++++++++-----
 drivers/mmc/core/queue.c | 14 +++++++++++---
 2 files changed, 23 insertions(+), 8 deletions(-)

-- 
2.21.0.1020.gf2820cf01a-goog

[stable/4.14.y PATCH 1/3] mmc: block: Simplify cleaning up the queue

[Raul E Rangel]

From: Adrian Hunter <adrian.hunter@intel.com>

Use blk_cleanup_queue() to shutdown the queue when the driver is removed,
and instead get an extra reference to the queue to prevent the queue being
freed before the final mmc_blk_put().

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
commit 41e3efd07d5a02c80f503e29d755aa1bbb4245de upstream.

 drivers/mmc/core/block.c | 17 ++++++++++++-----
 drivers/mmc/core/queue.c |  2 ++
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index ce6dd49fbb98d..203038fb85111 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -161,7 +161,7 @@ static void mmc_blk_put(struct mmc_blk_data *md)
 	md->usage--;
 	if (md->usage == 0) {
 		int devidx = mmc_get_devidx(md->disk);
-		blk_cleanup_queue(md->queue.queue);
+		blk_put_queue(md->queue.queue);
 		ida_simple_remove(&mmc_blk_ida, devidx);
 		put_disk(md->disk);
 		kfree(md);
@@ -2122,6 +2122,17 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 
 	md->queue.blkdata = md;
 
+	/*
+	 * Keep an extra reference to the queue so that we can shutdown the
+	 * queue (i.e. call blk_cleanup_queue()) while there are still
+	 * references to the 'md'. The corresponding blk_put_queue() is in
+	 * mmc_blk_put().
+	 */
+	if (!blk_get_queue(md->queue.queue)) {
+		mmc_cleanup_queue(&md->queue);
+		goto err_putdisk;
+	}
+
 	md->disk->major	= MMC_BLOCK_MAJOR;
 	md->disk->first_minor = devidx * perdev_minors;
 	md->disk->fops = &mmc_bdops;
@@ -2272,10 +2283,6 @@ static void mmc_blk_remove_req(struct mmc_blk_data *md)
 		 * from being accepted.
 		 */
 		card = md->queue.card;
-		spin_lock_irq(md->queue.queue->queue_lock);
-		queue_flag_set(QUEUE_FLAG_BYPASS, md->queue.queue);
-		spin_unlock_irq(md->queue.queue->queue_lock);
-		blk_set_queue_dying(md->queue.queue);
 		mmc_cleanup_queue(&md->queue);
 		if (md->disk->flags & GENHD_FL_UP) {
 			device_remove_file(disk_to_dev(md->disk), &md->force_ro);
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index 0a4e77a5ba33f..d99fa4e63033c 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -259,6 +259,8 @@ void mmc_cleanup_queue(struct mmc_queue *mq)
 	blk_start_queue(q);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
+	blk_cleanup_queue(q);
+
 	mq->card = NULL;
 }
 EXPORT_SYMBOL(mmc_cleanup_queue);
-- 
2.21.0.1020.gf2820cf01a-goog

[stable/4.14.y PATCH 2/3] mmc: Fix null pointer dereference in mmc_init_request

[Raul E Rangel]

It is possible for queuedata to be cleared in mmc_cleanup_queue before
the request has been started. This will result in dereferencing a null
pointer.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/mmc/core/queue.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index d99fa4e63033c..bd7d521d5ad9d 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -159,8 +159,14 @@ static int mmc_init_request(struct request_queue *q, struct request *req,
 {
 	struct mmc_queue_req *mq_rq = req_to_mmc_queue_req(req);
 	struct mmc_queue *mq = q->queuedata;
-	struct mmc_card *card = mq->card;
-	struct mmc_host *host = card->host;
+	struct mmc_card *card;
+	struct mmc_host *host;
+
+	if (!mq)
+		return -ENODEV;
+
+	card = mq->card;
+	host = card->host;
 
 	mq_rq->sg = mmc_alloc_sg(host->max_segs, gfp);
 	if (!mq_rq->sg)
-- 
2.21.0.1020.gf2820cf01a-goog

Re: [stable/4.14.y PATCH 2/3] mmc: Fix null pointer dereference in mmc_init_request

[Greg KH]

On Mon, May 13, 2019 at 11:55:20AM -0600, Raul E Rangel wrote:
> It is possible for queuedata to be cleared in mmc_cleanup_queue before
> the request has been started. This will result in dereferencing a null
> pointer.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
> 
>  drivers/mmc/core/queue.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

[stable/4.14.y PATCH 3/3] mmc: Kill the request if the queuedata has been removed

[Raul E Rangel]

No reason to even try processing the request if the queue is shutting
down.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/mmc/core/queue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index bd7d521d5ad9d..e7ac7163fafa4 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -30,7 +30,7 @@ static int mmc_prep_request(struct request_queue *q, struct request *req)
 {
 	struct mmc_queue *mq = q->queuedata;
 
-	if (mq && (mmc_card_removed(mq->card) || mmc_access_rpmb(mq)))
+	if (!mq || mmc_card_removed(mq->card) || mmc_access_rpmb(mq))
 		return BLKPREP_KILL;
 
 	req->rq_flags |= RQF_DONTPREP;
-- 
2.21.0.1020.gf2820cf01a-goog

Re: [stable/4.14.y PATCH 3/3] mmc: Kill the request if the queuedata has been removed

[Greg KH]

On Mon, May 13, 2019 at 11:55:21AM -0600, Raul E Rangel wrote:
> No reason to even try processing the request if the queue is shutting
> down.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
> 
>  drivers/mmc/core/queue.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [stable/4.14.y PATCH 0/3] mmc: Fix a potential resource leak when shutting down request queue.

[Sasha Levin]

On Mon, May 13, 2019 at 11:55:18AM -0600, Raul E Rangel wrote:
>I think we should cherry-pick 41e3efd07d5a02c80f503e29d755aa1bbb4245de
>https://lore.kernel.org/patchwork/patch/856512/ into 4.14. It fixes a
>potential resource leak when shutting down the request queue.
>
>Once this patch is applied, there is a potential for a null pointer dereference.
>That's what the second patch fixes.
>
>The third patch is just an optimization to stop processing earlier.

Is this actually part of a fix? Why do we want this optimization?

--
Thanks,
Sasha

Re: [stable/4.14.y PATCH 0/3] mmc: Fix a potential resource leak when shutting down request queue.

[Greg Kroah-Hartman]

On Mon, May 13, 2019 at 11:55:18AM -0600, Raul E Rangel wrote:
> I think we should cherry-pick 41e3efd07d5a02c80f503e29d755aa1bbb4245de
> https://lore.kernel.org/patchwork/patch/856512/ into 4.14. It fixes a
> potential resource leak when shutting down the request queue.

Potential meaning "it does happen", or "it can happen if we do this", or
just "maybe it might happen, we really do not know?"

> Once this patch is applied, there is a potential for a null pointer dereference.
> That's what the second patch fixes.

What is the git id of that upstream fix?

> The third patch is just an optimization to stop processing earlier.

That's not how stable kernels work :(

> See https://patchwork.kernel.org/patch/10925469/ for the initial motivation.

I don't understand the motivation from that link at all :(

> This commit applies to v4.14.116. It is already included in 4.19. 4.19 doesn't
> suffer from the null pointer dereference because later commits migrate the mmc
> stack to blk-mq.

What are those later commits?

> I tested this patch set by randomly connecting/disconnecting the SD
> card. I got over 189650 itarations without a problem.

And if you do not have these patches, on 4.14.y, how many iterations
cause a problem?  If you just apply the first patch, does that work?

_EVERY_ time we take a patch that is not upstream, something usually is
broken and needs to be fixed.  We have a long long long history of this,
so if you want to have a patch that is not upstream applied to a stable
kernel release, you need a whole lot of justification and explanation
and begging.  And you need to be around to fix the fallout for when it
breaks :)

thanks,

greg k-h

Re: [stable/4.14.y PATCH 0/3] mmc: Fix a potential resource leak when shutting down request queue.

[Raul Rangel]

On Tue, May 14, 2019 at 11:19:34AM +0200, Greg Kroah-Hartman wrote:
> On Mon, May 13, 2019 at 11:55:18AM -0600, Raul E Rangel wrote:
> > I think we should cherry-pick 41e3efd07d5a02c80f503e29d755aa1bbb4245de
> > https://lore.kernel.org/patchwork/patch/856512/ into 4.14. It fixes a
> > potential resource leak when shutting down the request queue.
> 
> Potential meaning "it does happen", or "it can happen if we do this", or
> just "maybe it might happen, we really do not know?"
It does happen if the AMD SDHCI patches are cherry-picked into 4.14.
https://lkml.org/lkml/2019/5/1/398
It can be mitigated by changing the line in the patch with
`mmc_detect_change(host, 0)` to mmc_detect_change(host, 200)`, but
that's just a workaround to play with the timing so the race condition
doesn't happen.
> 
> > Once this patch is applied, there is a potential for a null pointer dereference.
> > That's what the second patch fixes.
> 
> What is the git id of that upstream fix?
So there is no specific upstream fix. There was a large patch set that
migrated mmc to using blk-mq, so the bug just kind of went away.
https://lwn.net/Articles/739774/ or 0fbfd12518303e9b32ac9fd231439459eac848f9

> 
> > The third patch is just an optimization to stop processing earlier.
> 
> That's not how stable kernels work :(
Oops, I guess we can ignore that patch. It just prevents mmc_init_request
from being called, but it doesn't matter since the 2nd patch actually
checks for NULL now.
> 
> > See https://patchwork.kernel.org/patch/10925469/ for the initial motivation.
> 
> I don't understand the motivation from that link at all :(
> 
> > This commit applies to v4.14.116. It is already included in 4.19. 4.19 doesn't
> > suffer from the null pointer dereference because later commits migrate the mmc
> > stack to blk-mq.
> 
> What are those later commits?
Commit 0fbfd12518303e9b32ac9fd231439459eac848f9 specifically deletes the
code for the 2nd patch. As I said above, the NULL pointer dereference
just kind of went away as part of the blk-mq migration, so there is no
upstream fix :(

> 
> > I tested this patch set by randomly connecting/disconnecting the SD
> > card. I got over 189650 itarations without a problem.
> 
> And if you do not have these patches, on 4.14.y, how many iterations
> cause a problem?  If you just apply the first patch, does that work?
If I apply the AMD SDHCI patches and nothing else, then I can cause a
resource leak within 10 iterations. If I apply just the first patch then
I can cause a NULL pointer error within 10 iterations. If I apply both 1
and 2, then everything works as expected and I can't cause a problem.
> 
> _EVERY_ time we take a patch that is not upstream, something usually is
> broken and needs to be fixed.  We have a long long long history of this,
> so if you want to have a patch that is not upstream applied to a stable
> kernel release, you need a whole lot of justification and explanation
> and begging.  And you need to be around to fix the fallout for when it
> breaks :)

It also looks like 2361bfb055f948eac6583fa3c75a014da84fe554 includes a
fix for 41e3efd07d5a02c80f503e29d755aa1bbb4245de, so that would need to
be cherry picked in.

I guess I should have included a fixes: line in my second patch.

So to summarize:
- cherry-pick 41e3efd07d5a02c80f503e29d755aa1bbb4245de
- cherry-pick 2361bfb055f948eac6583fa3c75a014da84fe554
- apply 2nd patch but add to commit message:
  Fixes: 41e3efd07d5a ("mmc: block: Simplify cleaning up the queue")
- Ignore patch 3 since it's an optimization.
> 
> thanks,
> 
> greg k-h
Thanks again for your time, and sorry for the really late response!

Raul

Re: [stable/4.14.y PATCH 0/3] mmc: Fix a potential resource leak when shutting down request queue.

[Greg Kroah-Hartman]

On Wed, Jun 19, 2019 at 10:46:25AM -0600, Raul Rangel wrote:
> On Tue, May 14, 2019 at 11:19:34AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, May 13, 2019 at 11:55:18AM -0600, Raul E Rangel wrote:
> > > I think we should cherry-pick 41e3efd07d5a02c80f503e29d755aa1bbb4245de
> > > https://lore.kernel.org/patchwork/patch/856512/ into 4.14. It fixes a
> > > potential resource leak when shutting down the request queue.
> > 
> > Potential meaning "it does happen", or "it can happen if we do this", or
> > just "maybe it might happen, we really do not know?"
> It does happen if the AMD SDHCI patches are cherry-picked into 4.14.
> https://lkml.org/lkml/2019/5/1/398

Why are those patches somehow being required to be added to 4.14.y?  If
they are not added, is all fine?

thanks,

greg k-h

Re: [stable/4.14.y PATCH 0/3] mmc: Fix a potential resource leak when shutting down request queue.

[Raul Rangel]

On Wed, Jun 19, 2019 at 07:09:17PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 19, 2019 at 10:46:25AM -0600, Raul Rangel wrote:
> > On Tue, May 14, 2019 at 11:19:34AM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, May 13, 2019 at 11:55:18AM -0600, Raul E Rangel wrote:
> > > > I think we should cherry-pick 41e3efd07d5a02c80f503e29d755aa1bbb4245de
> > > > https://lore.kernel.org/patchwork/patch/856512/ into 4.14. It fixes a
> > > > potential resource leak when shutting down the request queue.
> > > 
> > > Potential meaning "it does happen", or "it can happen if we do this", or
> > > just "maybe it might happen, we really do not know?"
> > It does happen if the AMD SDHCI patches are cherry-picked into 4.14.
> > https://lkml.org/lkml/2019/5/1/398
> 
> Why are those patches somehow being required to be added to 4.14.y?  If
> they are not added, is all fine?
I was just thinking we would backport the patches to fix this AMD SDHCI
hardware bug, but I guess we don't need to.

Thanks

Re: [stable/4.14.y PATCH 0/3] mmc: Fix a potential resource leak when shutting down request queue.

[Greg Kroah-Hartman]

On Wed, Jun 19, 2019 at 12:23:04PM -0600, Raul Rangel wrote:
> On Wed, Jun 19, 2019 at 07:09:17PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Jun 19, 2019 at 10:46:25AM -0600, Raul Rangel wrote:
> > > On Tue, May 14, 2019 at 11:19:34AM +0200, Greg Kroah-Hartman wrote:
> > > > On Mon, May 13, 2019 at 11:55:18AM -0600, Raul E Rangel wrote:
> > > > > I think we should cherry-pick 41e3efd07d5a02c80f503e29d755aa1bbb4245de
> > > > > https://lore.kernel.org/patchwork/patch/856512/ into 4.14. It fixes a
> > > > > potential resource leak when shutting down the request queue.
> > > > 
> > > > Potential meaning "it does happen", or "it can happen if we do this", or
> > > > just "maybe it might happen, we really do not know?"
> > > It does happen if the AMD SDHCI patches are cherry-picked into 4.14.
> > > https://lkml.org/lkml/2019/5/1/398
> > 
> > Why are those patches somehow being required to be added to 4.14.y?  If
> > they are not added, is all fine?
> I was just thinking we would backport the patches to fix this AMD SDHCI
> hardware bug, but I guess we don't need to.

Has anyone asked for those to be backported?  Does anyone require them
to be?  What's keeping users from using a newer kernel that have this
specific hardware issue?

Trying to apply patches to a stable kernel due to an issue that is not
even in that stable kernel is crazy.  No wonder I am totally confused...

thanks,

greg k-h