[PATCH] mmc: core: don't initialize block size from ext_csd if not present

[PATCH] mmc: core: don't initialize block size from ext_csd if not present

[Peter Collingbourne]

If extended CSD was not available, the eMMC driver would incorrectly
set the block size to 0, as the data_sector_size field of ext_csd
was never initialized. This issue was exposed by commit 817046ecddbc
("block: Align max_hw_sectors to logical blocksize") which caused
max_sectors and max_hw_sectors to be set to 0 after setting the block
size to 0, resulting in a kernel panic in bio_split when attempting
to read from the device. Fix it by only reading the block size from
ext_csd if it is available.

Fixes: 817046ecddbc ("block: Align max_hw_sectors to logical blocksize")
Signed-off-by: Peter Collingbourne <pcc@google.com>
Link: https://linux-review.googlesource.com/id/If244d178da4d86b52034459438fec295b02d6e60
---
 drivers/mmc/core/queue.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index de7cb0369c30..735cdbf1145c 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -20,6 +20,7 @@
 #include "core.h"
 #include "card.h"
 #include "host.h"
+#include "mmc_ops.h"
 
 #define MMC_DMA_MAP_MERGE_SEGMENTS	512
 
@@ -384,7 +385,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
 		     "merging was advertised but not possible");
 	blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
 
-	if (mmc_card_mmc(card))
+	if (mmc_card_mmc(card) && mmc_can_ext_csd(card))
 		block_size = card->ext_csd.data_sector_size;
 
 	blk_queue_logical_block_size(mq->queue, block_size);
-- 
2.30.0.284.gd98b1dd5eaa7-goog

Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present

[Damien Le Moal]

On 2021/01/13 6:09, Peter Collingbourne wrote:
> If extended CSD was not available, the eMMC driver would incorrectly
> set the block size to 0, as the data_sector_size field of ext_csd
> was never initialized. This issue was exposed by commit 817046ecddbc
> ("block: Align max_hw_sectors to logical blocksize") which caused
> max_sectors and max_hw_sectors to be set to 0 after setting the block
> size to 0, resulting in a kernel panic in bio_split when attempting
> to read from the device. Fix it by only reading the block size from
> ext_csd if it is available.
> 
> Fixes: 817046ecddbc ("block: Align max_hw_sectors to logical blocksize")
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/If244d178da4d86b52034459438fec295b02d6e60
> ---
>  drivers/mmc/core/queue.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index de7cb0369c30..735cdbf1145c 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -20,6 +20,7 @@
>  #include "core.h"
>  #include "card.h"
>  #include "host.h"
> +#include "mmc_ops.h"
>  
>  #define MMC_DMA_MAP_MERGE_SEGMENTS	512
>  
> @@ -384,7 +385,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
>  		     "merging was advertised but not possible");
>  	blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
>  
> -	if (mmc_card_mmc(card))
> +	if (mmc_card_mmc(card) && mmc_can_ext_csd(card))
>  		block_size = card->ext_csd.data_sector_size;
>  
>  	blk_queue_logical_block_size(mq->queue, block_size);
> 

This looks good to me. I wonder though if we should add a WARN() in
blk_queue_logical_block_size() if it is passed a 0 block size to more easily
catch this kind of bug.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

Hi Peter,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on hch-configfs/for-next v5.11-rc3 next-20210113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Peter-Collingbourne/mmc-core-don-t-initialize-block-size-from-ext_csd-if-not-present/20210113-054414
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e609571b5ffa3528bf85292de1ceaddac342bc1c
config: x86_64-rhel (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/562a77dc6c88601c5129c0bbb9141119494320c7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Peter-Collingbourne/mmc-core-don-t-initialize-block-size-from-ext_csd-if-not-present/20210113-054414
        git checkout 562a77dc6c88601c5129c0bbb9141119494320c7
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "mmc_can_ext_csd" [drivers/mmc/core/mmc_block.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45904 bytes --]

Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present

[Adrian Hunter]

On 12/01/21 11:09 pm, Peter Collingbourne wrote:
> If extended CSD was not available, the eMMC driver would incorrectly
> set the block size to 0, as the data_sector_size field of ext_csd
> was never initialized. This issue was exposed by commit 817046ecddbc
> ("block: Align max_hw_sectors to logical blocksize") which caused
> max_sectors and max_hw_sectors to be set to 0 after setting the block
> size to 0, resulting in a kernel panic in bio_split when attempting
> to read from the device. Fix it by only reading the block size from
> ext_csd if it is available.
> 
> Fixes: 817046ecddbc ("block: Align max_hw_sectors to logical blocksize")

I would go with the original commit i.e.

Fixes: a5075eb94837 ("mmc: block: Allow disabling 512B sector size emulation")

> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/If244d178da4d86b52034459438fec295b02d6e60
> ---
>  drivers/mmc/core/queue.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index de7cb0369c30..735cdbf1145c 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -20,6 +20,7 @@
>  #include "core.h"
>  #include "card.h"
>  #include "host.h"
> +#include "mmc_ops.h"
>  
>  #define MMC_DMA_MAP_MERGE_SEGMENTS	512
>  
> @@ -384,7 +385,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
>  		     "merging was advertised but not possible");
>  	blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
>  
> -	if (mmc_card_mmc(card))
> +	if (mmc_card_mmc(card) && mmc_can_ext_csd(card))
>  		block_size = card->ext_csd.data_sector_size;

Might as well be:

	if (mmc_card_mmc(card) && card->ext_csd.data_sector_size)
  		block_size = card->ext_csd.data_sector_size;



>  
>  	blk_queue_logical_block_size(mq->queue, block_size);
> 

Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present

[Peter Collingbourne]

On Wed, Jan 13, 2021 at 2:43 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 12/01/21 11:09 pm, Peter Collingbourne wrote:
> > If extended CSD was not available, the eMMC driver would incorrectly
> > set the block size to 0, as the data_sector_size field of ext_csd
> > was never initialized. This issue was exposed by commit 817046ecddbc
> > ("block: Align max_hw_sectors to logical blocksize") which caused
> > max_sectors and max_hw_sectors to be set to 0 after setting the block
> > size to 0, resulting in a kernel panic in bio_split when attempting
> > to read from the device. Fix it by only reading the block size from
> > ext_csd if it is available.
> >
> > Fixes: 817046ecddbc ("block: Align max_hw_sectors to logical blocksize")
>
> I would go with the original commit i.e.
>
> Fixes: a5075eb94837 ("mmc: block: Allow disabling 512B sector size emulation")

Sure, makes sense.

> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > Link: https://linux-review.googlesource.com/id/If244d178da4d86b52034459438fec295b02d6e60
> > ---
> >  drivers/mmc/core/queue.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> > index de7cb0369c30..735cdbf1145c 100644
> > --- a/drivers/mmc/core/queue.c
> > +++ b/drivers/mmc/core/queue.c
> > @@ -20,6 +20,7 @@
> >  #include "core.h"
> >  #include "card.h"
> >  #include "host.h"
> > +#include "mmc_ops.h"
> >
> >  #define MMC_DMA_MAP_MERGE_SEGMENTS   512
> >
> > @@ -384,7 +385,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
> >                    "merging was advertised but not possible");
> >       blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
> >
> > -     if (mmc_card_mmc(card))
> > +     if (mmc_card_mmc(card) && mmc_can_ext_csd(card))
> >               block_size = card->ext_csd.data_sector_size;
>
> Might as well be:
>
>         if (mmc_card_mmc(card) && card->ext_csd.data_sector_size)
>                 block_size = card->ext_csd.data_sector_size;

Can we rely on this data structure to be zero initialized? I suppose
so, provided that it was allocated with mmc_alloc_card which uses
kzalloc. But it isn't entirely obvious and I figure it may be a little
better to be explicit in our intent here. But either way works for me.

Peter

Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present

[Adrian Hunter]

On 13/01/21 9:46 pm, Peter Collingbourne wrote:
> On Wed, Jan 13, 2021 at 2:43 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 12/01/21 11:09 pm, Peter Collingbourne wrote:
>>> If extended CSD was not available, the eMMC driver would incorrectly
>>> set the block size to 0, as the data_sector_size field of ext_csd
>>> was never initialized. This issue was exposed by commit 817046ecddbc
>>> ("block: Align max_hw_sectors to logical blocksize") which caused
>>> max_sectors and max_hw_sectors to be set to 0 after setting the block
>>> size to 0, resulting in a kernel panic in bio_split when attempting
>>> to read from the device. Fix it by only reading the block size from
>>> ext_csd if it is available.
>>>
>>> Fixes: 817046ecddbc ("block: Align max_hw_sectors to logical blocksize")
>>
>> I would go with the original commit i.e.
>>
>> Fixes: a5075eb94837 ("mmc: block: Allow disabling 512B sector size emulation")
> 
> Sure, makes sense.
> 
>>> Signed-off-by: Peter Collingbourne <pcc@google.com>
>>> Link: https://linux-review.googlesource.com/id/If244d178da4d86b52034459438fec295b02d6e60
>>> ---
>>>  drivers/mmc/core/queue.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
>>> index de7cb0369c30..735cdbf1145c 100644
>>> --- a/drivers/mmc/core/queue.c
>>> +++ b/drivers/mmc/core/queue.c
>>> @@ -20,6 +20,7 @@
>>>  #include "core.h"
>>>  #include "card.h"
>>>  #include "host.h"
>>> +#include "mmc_ops.h"
>>>
>>>  #define MMC_DMA_MAP_MERGE_SEGMENTS   512
>>>
>>> @@ -384,7 +385,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
>>>                    "merging was advertised but not possible");
>>>       blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
>>>
>>> -     if (mmc_card_mmc(card))
>>> +     if (mmc_card_mmc(card) && mmc_can_ext_csd(card))
>>>               block_size = card->ext_csd.data_sector_size;
>>
>> Might as well be:
>>
>>         if (mmc_card_mmc(card) && card->ext_csd.data_sector_size)
>>                 block_size = card->ext_csd.data_sector_size;
> 
> Can we rely on this data structure to be zero initialized? I suppose
> so, provided that it was allocated with mmc_alloc_card which uses
> kzalloc. But it isn't entirely obvious and I figure it may be a little
> better to be explicit in our intent here. But either way works for me.

The only valid values are 512 and 4096, so you could add WARN_ON(block_size
!= 512 && block_size != 4096) if you want.

Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present

[Peter Collingbourne]

On Wed, Jan 13, 2021 at 10:11 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 13/01/21 9:46 pm, Peter Collingbourne wrote:
> > On Wed, Jan 13, 2021 at 2:43 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 12/01/21 11:09 pm, Peter Collingbourne wrote:
> >>> If extended CSD was not available, the eMMC driver would incorrectly
> >>> set the block size to 0, as the data_sector_size field of ext_csd
> >>> was never initialized. This issue was exposed by commit 817046ecddbc
> >>> ("block: Align max_hw_sectors to logical blocksize") which caused
> >>> max_sectors and max_hw_sectors to be set to 0 after setting the block
> >>> size to 0, resulting in a kernel panic in bio_split when attempting
> >>> to read from the device. Fix it by only reading the block size from
> >>> ext_csd if it is available.
> >>>
> >>> Fixes: 817046ecddbc ("block: Align max_hw_sectors to logical blocksize")
> >>
> >> I would go with the original commit i.e.
> >>
> >> Fixes: a5075eb94837 ("mmc: block: Allow disabling 512B sector size emulation")
> >
> > Sure, makes sense.
> >
> >>> Signed-off-by: Peter Collingbourne <pcc@google.com>
> >>> Link: https://linux-review.googlesource.com/id/If244d178da4d86b52034459438fec295b02d6e60
> >>> ---
> >>>  drivers/mmc/core/queue.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> >>> index de7cb0369c30..735cdbf1145c 100644
> >>> --- a/drivers/mmc/core/queue.c
> >>> +++ b/drivers/mmc/core/queue.c
> >>> @@ -20,6 +20,7 @@
> >>>  #include "core.h"
> >>>  #include "card.h"
> >>>  #include "host.h"
> >>> +#include "mmc_ops.h"
> >>>
> >>>  #define MMC_DMA_MAP_MERGE_SEGMENTS   512
> >>>
> >>> @@ -384,7 +385,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
> >>>                    "merging was advertised but not possible");
> >>>       blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
> >>>
> >>> -     if (mmc_card_mmc(card))
> >>> +     if (mmc_card_mmc(card) && mmc_can_ext_csd(card))
> >>>               block_size = card->ext_csd.data_sector_size;
> >>
> >> Might as well be:
> >>
> >>         if (mmc_card_mmc(card) && card->ext_csd.data_sector_size)
> >>                 block_size = card->ext_csd.data_sector_size;
> >
> > Can we rely on this data structure to be zero initialized? I suppose
> > so, provided that it was allocated with mmc_alloc_card which uses
> > kzalloc. But it isn't entirely obvious and I figure it may be a little
> > better to be explicit in our intent here. But either way works for me.
>
> The only valid values are 512 and 4096, so you could add WARN_ON(block_size
> != 512 && block_size != 4096) if you want.

Okay, I did that in v2.

Peter