* [PATCH] mmc: card: move variable initialization earlier @ 2012-03-23 9:32 Linus Walleij 2012-04-01 4:07 ` Chris Ball 0 siblings, 1 reply; 6+ messages in thread From: Linus Walleij @ 2012-03-23 9:32 UTC (permalink / raw) To: linux-mmc, Chris Ball; +Cc: Linus Walleij, Ulf Hansson, Rabin Vincent I was pretty tired of seeing these in my kernel compiles: drivers/mmc/card/block.c: In function ‘mmc_blk_issue_secdiscard_rq’: drivers/mmc/card/block.c:911:18: warning: ‘arg’ may be used uninitialized in this function [-Wuninitialized] drivers/mmc/card/block.c:910:6: warning: ‘nr’ may be used uninitialized in this function [-Wuninitialized] drivers/mmc/card/block.c:910:6: warning: ‘from’ may be used uninitialized in this function [-Wuninitialized] The problem stems from the code path in mmc_blk_issue_secdiscard_rq() where mmc_switch() with EXT_CSD_SANITIZE_START may return -EIO and fall back to using the default trim operations instead. At this point the variables needed for the fallback will be uninitialized. Cc: Ulf Hansson <ulf.hansson@stericsson.com> Cc: Rabin Vincent <rabin@rab.in> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- I don't know if this is the actual intention - maybe we should just fail the call entirely if the sanitize command fails? --- drivers/mmc/card/block.c | 15 ++++++++------- 1 files changed, 8 insertions(+), 7 deletions(-) diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index c6a383d..2741d93 100644 --- a/drivers/mmc/card/block.c +++ b/drivers/mmc/card/block.c @@ -877,6 +877,14 @@ static int mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq, unsigned int from, nr, arg; int err = 0, type = MMC_BLK_SECDISCARD; + from = blk_rq_pos(req); + nr = blk_rq_sectors(req); + + if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr)) + arg = MMC_SECURE_TRIM1_ARG; + else + arg = MMC_SECURE_ERASE_ARG; + if (!(mmc_can_secure_erase_trim(card) || mmc_can_sanitize(card))) { err = -EOPNOTSUPP; goto out; @@ -889,13 +897,6 @@ static int mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq, goto out; } - from = blk_rq_pos(req); - nr = blk_rq_sectors(req); - - if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr)) - arg = MMC_SECURE_TRIM1_ARG; - else - arg = MMC_SECURE_ERASE_ARG; retry: if (card->quirks & MMC_QUIRK_INAND_CMD38) { err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: card: move variable initialization earlier 2012-03-23 9:32 [PATCH] mmc: card: move variable initialization earlier Linus Walleij @ 2012-04-01 4:07 ` Chris Ball 2012-04-03 8:40 ` Adrian Hunter 0 siblings, 1 reply; 6+ messages in thread From: Chris Ball @ 2012-04-01 4:07 UTC (permalink / raw) To: Linus Walleij; +Cc: linux-mmc, Ulf Hansson, Rabin Vincent, Adrian Hunter Hi Adrian, On Fri, Mar 23 2012, Linus Walleij wrote: > I was pretty tired of seeing these in my kernel compiles: > > drivers/mmc/card/block.c: In function ‘mmc_blk_issue_secdiscard_rq’: > drivers/mmc/card/block.c:911:18: warning: ‘arg’ may be used uninitialized in this function [-Wuninitialized] > drivers/mmc/card/block.c:910:6: warning: ‘nr’ may be used uninitialized in this function [-Wuninitialized] > drivers/mmc/card/block.c:910:6: warning: ‘from’ may be used uninitialized in this function [-Wuninitialized] > > The problem stems from the code path in > mmc_blk_issue_secdiscard_rq() where mmc_switch() > with EXT_CSD_SANITIZE_START may return -EIO and fall back > to using the default trim operations instead. At this point > the variables needed for the fallback will be uninitialized. > > Cc: Ulf Hansson <ulf.hansson@stericsson.com> > Cc: Rabin Vincent <rabin@rab.in> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > I don't know if this is the actual intention - maybe we > should just fail the call entirely if the sanitize command > fails? I think you (Adrian) introduced this "goto out->goto retry" logic in upstream commit 67716327eec7e9 -- please could you take a look here? > --- > drivers/mmc/card/block.c | 15 ++++++++------- > 1 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c > index c6a383d..2741d93 100644 > --- a/drivers/mmc/card/block.c > +++ b/drivers/mmc/card/block.c > @@ -877,6 +877,14 @@ static int mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq, > unsigned int from, nr, arg; > int err = 0, type = MMC_BLK_SECDISCARD; > > + from = blk_rq_pos(req); > + nr = blk_rq_sectors(req); > + > + if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr)) > + arg = MMC_SECURE_TRIM1_ARG; > + else > + arg = MMC_SECURE_ERASE_ARG; > + > if (!(mmc_can_secure_erase_trim(card) || mmc_can_sanitize(card))) { > err = -EOPNOTSUPP; > goto out; > @@ -889,13 +897,6 @@ static int mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq, > goto out; > } > > - from = blk_rq_pos(req); > - nr = blk_rq_sectors(req); > - > - if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr)) > - arg = MMC_SECURE_TRIM1_ARG; > - else > - arg = MMC_SECURE_ERASE_ARG; > retry: > if (card->quirks & MMC_QUIRK_INAND_CMD38) { > err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, Thanks, - Chris. -- Chris Ball <cjb@laptop.org> <http://printf.net/> One Laptop Per Child ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: card: move variable initialization earlier 2012-04-01 4:07 ` Chris Ball @ 2012-04-03 8:40 ` Adrian Hunter 2012-04-03 10:57 ` Linus Walleij 2012-04-03 16:55 ` Luca Porzio (lporzio) 0 siblings, 2 replies; 6+ messages in thread From: Adrian Hunter @ 2012-04-03 8:40 UTC (permalink / raw) To: Chris Ball Cc: Linus Walleij, linux-mmc, Ulf Hansson, Rabin Vincent, Kyungmin Park, Kyungmin Park, Jaehoon Chung On 01/04/12 07:07, Chris Ball wrote: > Hi Adrian, > > On Fri, Mar 23 2012, Linus Walleij wrote: >> I was pretty tired of seeing these in my kernel compiles: >> >> drivers/mmc/card/block.c: In function ‘mmc_blk_issue_secdiscard_rq’: >> drivers/mmc/card/block.c:911:18: warning: ‘arg’ may be used uninitialized in this function [-Wuninitialized] >> drivers/mmc/card/block.c:910:6: warning: ‘nr’ may be used uninitialized in this function [-Wuninitialized] >> drivers/mmc/card/block.c:910:6: warning: ‘from’ may be used uninitialized in this function [-Wuninitialized] >> >> The problem stems from the code path in >> mmc_blk_issue_secdiscard_rq() where mmc_switch() >> with EXT_CSD_SANITIZE_START may return -EIO and fall back >> to using the default trim operations instead. At this point >> the variables needed for the fallback will be uninitialized. >> >> Cc: Ulf Hansson <ulf.hansson@stericsson.com> >> Cc: Rabin Vincent <rabin@rab.in> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> >> --- >> I don't know if this is the actual intention - maybe we >> should just fail the call entirely if the sanitize command >> fails? > > I think you (Adrian) introduced this "goto out->goto retry" logic in > upstream commit 67716327eec7e9 -- please could you take a look here? > The sanitize logic looks wrong to me. I would expect it to look like this: diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index b180965..f5e0534 100644 --- a/drivers/mmc/card/block.c +++ b/drivers/mmc/card/block.c @@ -881,17 +881,12 @@ static int mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq, goto out; } - /* The sanitize operation is supported at v4.5 only */ - if (mmc_can_sanitize(card)) { - err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, - EXT_CSD_SANITIZE_START, 1, 0); - goto out; - } - from = blk_rq_pos(req); nr = blk_rq_sectors(req); - if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr)) + if (mmc_can_sanitize(card)) + arg = MMC_DISCARD_ARG; + else if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr)) arg = MMC_SECURE_TRIM1_ARG; else arg = MMC_SECURE_ERASE_ARG; @@ -918,6 +913,12 @@ retry: } err = mmc_erase(card, from, nr, MMC_SECURE_TRIM2_ARG); } + + /* The sanitize operation is supported at v4.5 only */ + if (!err && mmc_can_sanitize(card)) { + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, + EXT_CSD_SANITIZE_START, 1, 0); + } out: if (err == -EIO && !mmc_blk_reset(md, card->host, type)) goto retry; Also the timeout for eMMC v4.5 DISCARD looks wrong. It should be the same as TRIM: diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 14f262e..00fd7db 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1407,7 +1407,7 @@ static unsigned int mmc_mmc_erase_timeout(struct mmc_card *card, if (card->ext_csd.erase_group_def & 1) { /* High Capacity Erase Group Size uses HC timeouts */ - if (arg == MMC_TRIM_ARG) + if (arg == MMC_TRIM_ARG || arg == MMC_DISCARD_ARG) erase_timeout = card->ext_csd.trim_timeout; else erase_timeout = card->ext_csd.hc_erase_timeout; In addition eMMC v4.5 seems to indicate the use of the trim timeout irrespective of the setting of erase_group_def, so maybe it should be like this: diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 14f262e..4691a23 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1405,7 +1405,10 @@ static unsigned int mmc_mmc_erase_timeout(struct mmc_card *card, { unsigned int erase_timeout; - if (card->ext_csd.erase_group_def & 1) { + if (arg == MMC_DISCARD_ARG || + (arg == MMC_TRIM_ARG && card->ext_csd.rev >= 6)) { + erase_timeout = card->ext_csd.trim_timeout; + } else if (card->ext_csd.erase_group_def & 1) { /* High Capacity Erase Group Size uses HC timeouts */ if (arg == MMC_TRIM_ARG) erase_timeout = card->ext_csd.trim_timeout; Alternatively, maybe it would be better to switch to HC erase size for all eMMC v4.5 cards? ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: card: move variable initialization earlier 2012-04-03 8:40 ` Adrian Hunter @ 2012-04-03 10:57 ` Linus Walleij 2012-04-03 16:55 ` Luca Porzio (lporzio) 1 sibling, 0 replies; 6+ messages in thread From: Linus Walleij @ 2012-04-03 10:57 UTC (permalink / raw) To: Adrian Hunter Cc: Chris Ball, linux-mmc, Ulf Hansson, Rabin Vincent, Kyungmin Park, Kyungmin Park, Jaehoon Chung On Tue, Apr 3, 2012 at 10:40 AM, Adrian Hunter <adrian.hunter@intel.com> wrote: >> I think you (Adrian) introduced this "goto out->goto retry" logic in >> upstream commit 67716327eec7e9 -- please could you take a look here? > > The sanitize logic looks wrong to me. I would expect it to look > like this: Hm it looks like that compile warning opens a can of worms. Can you cook a patch we can test? Would be much appreciated. Not that I have any eMMC 4.5 card, but I think some others on CC have... Yours, Linus Walleij ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] mmc: card: move variable initialization earlier 2012-04-03 8:40 ` Adrian Hunter 2012-04-03 10:57 ` Linus Walleij @ 2012-04-03 16:55 ` Luca Porzio (lporzio) 2012-04-04 6:20 ` Adrian Hunter 1 sibling, 1 reply; 6+ messages in thread From: Luca Porzio (lporzio) @ 2012-04-03 16:55 UTC (permalink / raw) To: Adrian Hunter, Chris Ball Cc: Linus Walleij, linux-mmc@vger.kernel.org, Ulf Hansson, Rabin Vincent, Kyungmin Park, Kyungmin Park, Jaehoon Chung Hi Adrian, > The sanitize logic looks wrong to me. I would expect it to look > like this: > > > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c > index b180965..f5e0534 100644 > --- a/drivers/mmc/card/block.c > +++ b/drivers/mmc/card/block.c > @@ -881,17 +881,12 @@ static int mmc_blk_issue_secdiscard_rq(struct mmc_queue > *mq, > goto out; > } > > - /* The sanitize operation is supported at v4.5 only */ > - if (mmc_can_sanitize(card)) { > - err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > - EXT_CSD_SANITIZE_START, 1, 0); > - goto out; > - } > - > from = blk_rq_pos(req); > nr = blk_rq_sectors(req); > > - if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr)) > + if (mmc_can_sanitize(card)) > + arg = MMC_DISCARD_ARG; The sanitize and discard are not coupled functionalities. Discard is a hint for performance meant to replace trim and erase where performance matters. Sanitize is a security operation meant to clear all unmapped contents. Jedec 4.5 spec does not guarantee that a discarded sector will be sanitized. This patch, if applied, will expose the kernel to a potential security risk (retrieve old contents not wiped by a sanitize) > + else if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr)) > arg = MMC_SECURE_TRIM1_ARG; > else > arg = MMC_SECURE_ERASE_ARG; > @@ -918,6 +913,12 @@ retry: > } > err = mmc_erase(card, from, nr, MMC_SECURE_TRIM2_ARG); > } > + > + /* The sanitize operation is supported at v4.5 only */ > + if (!err && mmc_can_sanitize(card)) { > + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > + EXT_CSD_SANITIZE_START, 1, 0); > + } > out: > if (err == -EIO && !mmc_blk_reset(md, card->host, type)) > goto retry; > > > > Also the timeout for eMMC v4.5 DISCARD looks wrong. It should be > the same as TRIM: > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 14f262e..00fd7db 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -1407,7 +1407,7 @@ static unsigned int mmc_mmc_erase_timeout(struct > mmc_card *card, > > if (card->ext_csd.erase_group_def & 1) { > /* High Capacity Erase Group Size uses HC timeouts */ > - if (arg == MMC_TRIM_ARG) > + if (arg == MMC_TRIM_ARG || arg == MMC_DISCARD_ARG) > erase_timeout = card->ext_csd.trim_timeout; > else > erase_timeout = card->ext_csd.hc_erase_timeout; > > Although I suspect that the discard cmd will be much faster than the Trim on most devices, there is no such info available as of today in ext csd. As such I agree with Adrian, discard timeout is nearer to trim than erase. > > In addition eMMC v4.5 seems to indicate the use of the trim timeout > irrespective of the setting of erase_group_def, so maybe it should be > like this: > > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 14f262e..4691a23 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -1405,7 +1405,10 @@ static unsigned int mmc_mmc_erase_timeout(struct > mmc_card *card, > { > unsigned int erase_timeout; > > - if (card->ext_csd.erase_group_def & 1) { > + if (arg == MMC_DISCARD_ARG || > + (arg == MMC_TRIM_ARG && card->ext_csd.rev >= 6)) { > + erase_timeout = card->ext_csd.trim_timeout; > + } else if (card->ext_csd.erase_group_def & 1) { > /* High Capacity Erase Group Size uses HC timeouts */ > if (arg == MMC_TRIM_ARG) > erase_timeout = card->ext_csd.trim_timeout; > > > > > Alternatively, maybe it would be better to switch to HC erase size for all > eMMC v4.5 cards? > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: card: move variable initialization earlier 2012-04-03 16:55 ` Luca Porzio (lporzio) @ 2012-04-04 6:20 ` Adrian Hunter 0 siblings, 0 replies; 6+ messages in thread From: Adrian Hunter @ 2012-04-04 6:20 UTC (permalink / raw) To: Luca Porzio (lporzio) Cc: Chris Ball, Linus Walleij, linux-mmc@vger.kernel.org, Ulf Hansson, Rabin Vincent, Kyungmin Park, Kyungmin Park, Jaehoon Chung On 03/04/12 19:55, Luca Porzio (lporzio) wrote: > Hi Adrian, > >> The sanitize logic looks wrong to me. I would expect it to look >> like this: >> >> >> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c >> index b180965..f5e0534 100644 >> --- a/drivers/mmc/card/block.c >> +++ b/drivers/mmc/card/block.c >> @@ -881,17 +881,12 @@ static int mmc_blk_issue_secdiscard_rq(struct mmc_queue >> *mq, >> goto out; >> } >> >> - /* The sanitize operation is supported at v4.5 only */ >> - if (mmc_can_sanitize(card)) { >> - err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >> - EXT_CSD_SANITIZE_START, 1, 0); >> - goto out; >> - } >> - >> from = blk_rq_pos(req); >> nr = blk_rq_sectors(req); >> >> - if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr)) >> + if (mmc_can_sanitize(card)) >> + arg = MMC_DISCARD_ARG; > > The sanitize and discard are not coupled functionalities. > Discard is a hint for performance meant to replace trim and erase > where performance matters. > Sanitize is a security operation meant to clear all unmapped contents. > Jedec 4.5 spec does not guarantee that a discarded sector will be sanitized. True, it is a bit vague on that point, although you might infer it from: The Sanitize operation is a feature, in addition to TRIM and Erase that is used to remove data from the device. which does not mention Discard. Presumably, some cards do make the guarantee e.g. Samsung since they submitted the original Discard/Sanitize patches. > This patch, if applied, will expose the kernel to a potential security > risk (retrieve old contents not wiped by a sanitize) Well, the kernel is already exposed. Current code does not even do a Discard if Sanitize is supported. > >> + else if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr)) >> arg = MMC_SECURE_TRIM1_ARG; >> else >> arg = MMC_SECURE_ERASE_ARG; >> @@ -918,6 +913,12 @@ retry: >> } >> err = mmc_erase(card, from, nr, MMC_SECURE_TRIM2_ARG); >> } >> + >> + /* The sanitize operation is supported at v4.5 only */ >> + if (!err && mmc_can_sanitize(card)) { >> + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >> + EXT_CSD_SANITIZE_START, 1, 0); >> + } >> out: >> if (err == -EIO && !mmc_blk_reset(md, card->host, type)) >> goto retry; >> >> >> >> Also the timeout for eMMC v4.5 DISCARD looks wrong. It should be >> the same as TRIM: >> >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index 14f262e..00fd7db 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -1407,7 +1407,7 @@ static unsigned int mmc_mmc_erase_timeout(struct >> mmc_card *card, >> >> if (card->ext_csd.erase_group_def & 1) { >> /* High Capacity Erase Group Size uses HC timeouts */ >> - if (arg == MMC_TRIM_ARG) >> + if (arg == MMC_TRIM_ARG || arg == MMC_DISCARD_ARG) >> erase_timeout = card->ext_csd.trim_timeout; >> else >> erase_timeout = card->ext_csd.hc_erase_timeout; >> >> > > Although I suspect that the discard cmd will be much faster than the > Trim on most devices, there is no such info available as of today in ext csd. > As such I agree with Adrian, discard timeout is nearer to trim than erase. > >> >> In addition eMMC v4.5 seems to indicate the use of the trim timeout >> irrespective of the setting of erase_group_def, so maybe it should be >> like this: >> >> >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index 14f262e..4691a23 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -1405,7 +1405,10 @@ static unsigned int mmc_mmc_erase_timeout(struct >> mmc_card *card, >> { >> unsigned int erase_timeout; >> >> - if (card->ext_csd.erase_group_def & 1) { >> + if (arg == MMC_DISCARD_ARG || >> + (arg == MMC_TRIM_ARG && card->ext_csd.rev >= 6)) { >> + erase_timeout = card->ext_csd.trim_timeout; >> + } else if (card->ext_csd.erase_group_def & 1) { >> /* High Capacity Erase Group Size uses HC timeouts */ >> if (arg == MMC_TRIM_ARG) >> erase_timeout = card->ext_csd.trim_timeout; >> >> >> >> >> Alternatively, maybe it would be better to switch to HC erase size for all >> eMMC v4.5 cards? >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-04-04 6:19 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-23 9:32 [PATCH] mmc: card: move variable initialization earlier Linus Walleij 2012-04-01 4:07 ` Chris Ball 2012-04-03 8:40 ` Adrian Hunter 2012-04-03 10:57 ` Linus Walleij 2012-04-03 16:55 ` Luca Porzio (lporzio) 2012-04-04 6:20 ` Adrian Hunter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).