[stable/4.14.y PATCH 0/3] mmc: Fix a potential resource leak when shutting down request queue.
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(-)
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);
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)
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>
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;
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>
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
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
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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Greg Kroah-Hartman
-
Raul E Rangel -
Raul Rangel
-
Sasha Levin