Re: [PATCH v4 2/2] mmc: sdio: deploy error handling instead of triggering BUG_ON

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 26 August 2016 at 02:49, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> wrote:
> When using mmc_io_rw_extended, it's intent to avoid null
> pointer of card and invalid func number. But actually it
> didn't prevent that as the seg_size already use the card.
> Currently the wrapper function sdio_io_rw_ext_helper already
> use card before calling mmc_io_rw_extended, so we should move
> this check to there. As to the func number, it was token from
> '(ocr & 0x70000000) >> 28' which should be enough to guarantee
> that it won't be larger than 7. But we should prevent the
> caller like wifi drivers modify this value. So let's move this
> check into sdio_io_rw_ext_helper either.
>
> Also we remove the BUG_ON for mmc_send_io_op_cond since all
> possible paths calling this function are protected by checking
> the arguments in advance. After deploying these changes, we
> could not see any panic within SDIO API even if func drivers
> abuse the SDIO func APIs.
>
> Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
>

Thanks, applied for next!

Kind regards
Uffe


> ---
>
> Changes in v4:
> - remove some unnecessary checks to simply code a bit more.
>
> Changes in v3:
> - remove more BUG_ONs for SDIO API and add error rountine
>   for either propagating error codes or casting warning before
>   return.
>
> Changes in v2: None
>
>  drivers/mmc/core/sdio_io.c  | 47 ++++++++++++++++++++++++++++++---------------
>  drivers/mmc/core/sdio_ops.c |  9 ++-------
>  2 files changed, 33 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
> index 78cb4d5..406e5f0 100644
> --- a/drivers/mmc/core/sdio_io.c
> +++ b/drivers/mmc/core/sdio_io.c
> @@ -26,8 +26,8 @@
>   */
>  void sdio_claim_host(struct sdio_func *func)
>  {
> -       BUG_ON(!func);
> -       BUG_ON(!func->card);
> +       if (WARN_ON(!func))
> +               return;
>
>         mmc_claim_host(func->card->host);
>  }
> @@ -42,8 +42,8 @@ EXPORT_SYMBOL_GPL(sdio_claim_host);
>   */
>  void sdio_release_host(struct sdio_func *func)
>  {
> -       BUG_ON(!func);
> -       BUG_ON(!func->card);
> +       if (WARN_ON(!func))
> +               return;
>
>         mmc_release_host(func->card->host);
>  }
> @@ -62,8 +62,8 @@ int sdio_enable_func(struct sdio_func *func)
>         unsigned char reg;
>         unsigned long timeout;
>
> -       BUG_ON(!func);
> -       BUG_ON(!func->card);
> +       if (!func)
> +               return -EINVAL;
>
>         pr_debug("SDIO: Enabling device %s...\n", sdio_func_id(func));
>
> @@ -112,8 +112,8 @@ int sdio_disable_func(struct sdio_func *func)
>         int ret;
>         unsigned char reg;
>
> -       BUG_ON(!func);
> -       BUG_ON(!func->card);
> +       if (!func)
> +               return -EINVAL;
>
>         pr_debug("SDIO: Disabling device %s...\n", sdio_func_id(func));
>
> @@ -307,6 +307,9 @@ static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
>         unsigned max_blocks;
>         int ret;
>
> +       if (!func || (func->num > 7))
> +               return -EINVAL;
> +
>         /* Do the bulk of the transfer using block mode (if supported). */
>         if (func->card->cccr.multi_block && (size > sdio_max_byte_size(func))) {
>                 /* Blocks per command is limited by host count, host transfer
> @@ -367,7 +370,10 @@ u8 sdio_readb(struct sdio_func *func, unsigned int addr, int *err_ret)
>         int ret;
>         u8 val;
>
> -       BUG_ON(!func);
> +       if (!func) {
> +               *err_ret = -EINVAL;
> +               return 0xFF;
> +       }
>
>         if (err_ret)
>                 *err_ret = 0;
> @@ -398,7 +404,10 @@ void sdio_writeb(struct sdio_func *func, u8 b, unsigned int addr, int *err_ret)
>  {
>         int ret;
>
> -       BUG_ON(!func);
> +       if (!func) {
> +               *err_ret = -EINVAL;
> +               return;
> +       }
>
>         ret = mmc_io_rw_direct(func->card, 1, func->num, addr, b, NULL);
>         if (err_ret)
> @@ -623,7 +632,10 @@ unsigned char sdio_f0_readb(struct sdio_func *func, unsigned int addr,
>         int ret;
>         unsigned char val;
>
> -       BUG_ON(!func);
> +       if (!func) {
> +               *err_ret = -EINVAL;
> +               return 0xFF;
> +       }
>
>         if (err_ret)
>                 *err_ret = 0;
> @@ -658,7 +670,10 @@ void sdio_f0_writeb(struct sdio_func *func, unsigned char b, unsigned int addr,
>  {
>         int ret;
>
> -       BUG_ON(!func);
> +       if (!func) {
> +               *err_ret = -EINVAL;
> +               return;
> +       }
>
>         if ((addr < 0xF0 || addr > 0xFF) && (!mmc_card_lenient_fn0(func->card))) {
>                 if (err_ret)
> @@ -684,8 +699,8 @@ EXPORT_SYMBOL_GPL(sdio_f0_writeb);
>   */
>  mmc_pm_flag_t sdio_get_host_pm_caps(struct sdio_func *func)
>  {
> -       BUG_ON(!func);
> -       BUG_ON(!func->card);
> +       if (!func)
> +               return 0;
>
>         return func->card->host->pm_caps;
>  }
> @@ -707,8 +722,8 @@ int sdio_set_host_pm_flags(struct sdio_func *func, mmc_pm_flag_t flags)
>  {
>         struct mmc_host *host;
>
> -       BUG_ON(!func);
> -       BUG_ON(!func->card);
> +       if (!func)
> +               return -EINVAL;
>
>         host = func->card->host;
>
> diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
> index 34f6e80..90fe554 100644
> --- a/drivers/mmc/core/sdio_ops.c
> +++ b/drivers/mmc/core/sdio_ops.c
> @@ -24,8 +24,6 @@ int mmc_send_io_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr)
>         struct mmc_command cmd = {0};
>         int i, err = 0;
>
> -       BUG_ON(!host);
> -
>         cmd.opcode = SD_IO_SEND_OP_COND;
>         cmd.arg = ocr;
>         cmd.flags = MMC_RSP_SPI_R4 | MMC_RSP_R4 | MMC_CMD_BCR;
> @@ -71,8 +69,8 @@ static int mmc_io_rw_direct_host(struct mmc_host *host, int write, unsigned fn,
>         struct mmc_command cmd = {0};
>         int err;
>
> -       BUG_ON(!host);
> -       BUG_ON(fn > 7);
> +       if (fn > 7)
> +               return -EINVAL;
>
>         /* sanity check */
>         if (addr & ~0x1FFFF)
> @@ -114,7 +112,6 @@ static int mmc_io_rw_direct_host(struct mmc_host *host, int write, unsigned fn,
>  int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
>         unsigned addr, u8 in, u8 *out)
>  {
> -       BUG_ON(!card);
>         return mmc_io_rw_direct_host(card->host, write, fn, addr, in, out);
>  }
>
> @@ -129,8 +126,6 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
>         unsigned int nents, left_size, i;
>         unsigned int seg_size = card->host->max_seg_size;
>
> -       BUG_ON(!card);
> -       BUG_ON(fn > 7);
>         WARN_ON(blksz == 0);
>
>         /* sanity check */
> --
> 2.3.7
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux