[PATCH] mtdinfo: don't open NULL pointer when getting region_info with `-a'

[PATCH] mtdinfo: don't open NULL pointer when getting region_info with `-a'

[Brian Norris]

This "fixes" a regression found in:
  commit 266061ebd5d72391f0a0e831b018e8fc7fea68a1
  mtdinfo: add regioninfo/eraseblock map display

On certain flash (NOR flash that have eraseblock region info),
`mtdinfo -a' tries to open the MTD node file, for use with the ioctl
MEMGETREGIONINFO; however, we didn't supply a device node path to
`mtdinfo -a', so it's using NULL, resulting in errors like:

  mtdinfo: error!: couldn't open MTD dev: (null)
           error 14 (Bad address)

For now, we can just skip dumping region_info with the `-a' flag. If
we find a better way to do this (e.g., export via sysfs, find device
nodes via automatic routines, etc.), then we can kill the workaround
and this FIXME should be removed.

The regression was first reported at:

  http://lists.infradead.org/pipermail/linux-mtd/2011-July/037232.html

Reported-by: Brian Foster <brian.foster@maxim-ic.com>
CC: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 ubi-utils/mtdinfo.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c
index e72d69e..f2fcd38 100644
--- a/ubi-utils/mtdinfo.c
+++ b/ubi-utils/mtdinfo.c
@@ -239,8 +239,14 @@ static void print_region_info(const struct mtd_dev_info *mtd)
 	region_info_t reginfo;
 	int r, fd;
 
-	/* If we don't have any region info, just return */
-	if (!args.map && mtd->region_cnt == 0)
+	/*
+	 * If we don't have any region info, just return
+	 *
+	 * FIXME: We can't get region_info (via ioctl) without having the MTD
+	 *        node path. This is a problem for `mtdinfo -a', for example,
+	 *        since it doesn't provide any filepath information.
+	 */
+	if (!args.node || !args.map && mtd->region_cnt == 0)
 		return;
 
 	/* First open the device so we can query it */
-- 
1.7.0.4

Re: [PATCH] mtdinfo: don't open NULL pointer when getting region_info with `-a'

[Mike Frysinger]

On Thu, Aug 4, 2011 at 10:47, Brian Norris wrote:
> This "fixes" a regression found in:
>  commit 266061ebd5d72391f0a0e831b018e8fc7fea68a1
>  mtdinfo: add regioninfo/eraseblock map display
>
> On certain flash (NOR flash that have eraseblock region info),
> `mtdinfo -a' tries to open the MTD node file, for use with the ioctl
> MEMGETREGIONINFO; however, we didn't supply a device node path to
> `mtdinfo -a', so it's using NULL, resulting in errors like:
>
>  mtdinfo: error!: couldn't open MTD dev: (null)
>           error 14 (Bad address)
>
> For now, we can just skip dumping region_info with the `-a' flag. If
> we find a better way to do this (e.g., export via sysfs, find device
> nodes via automatic routines, etc.), then we can kill the workaround
> and this FIXME should be removed.

the plan was to fixup the -a behavior once -m was punted.  we need to
rewrite it so that the main logic loops when all is being used and
takes care of constructing the device names via translate_dev.
-mike

Re: [PATCH] mtdinfo: don't open NULL pointer when getting region_info with `-a'

[Brian Foster]

On Thursday 04 August 2011 19:47:31 Brian Norris wrote:
> This "fixes" a regression found in:
>   commit 266061ebd5d72391f0a0e831b018e8fc7fea68a1
>   mtdinfo: add regioninfo/eraseblock map display
> 
> On certain flash (NOR flash that have eraseblock region info),
> `mtdinfo -a' tries to open the MTD node file, for use with the ioctl
> MEMGETREGIONINFO; however, we didn't supply a device node path to
> `mtdinfo -a', so it's using NULL, resulting in errors like:
> 
>   mtdinfo: error!: couldn't open MTD dev: (null)
>            error 14 (Bad address)
> 
> For now, we can just skip dumping region_info with the `-a' flag. If
> we find a better way to do this (e.g., export via sysfs, find device
> nodes via automatic routines, etc.), then we can kill the workaround
> and this FIXME should be removed.
> 
> The regression was first reported at:
> 
>   http://lists.infradead.org/pipermail/linux-mtd/2011-July/037232.html
> 
> Reported-by: Brian Foster <brian.foster@maxim-ic.com>
> CC: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  ubi-utils/mtdinfo.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c
> index e72d69e..f2fcd38 100644
> --- a/ubi-utils/mtdinfo.c
> +++ b/ubi-utils/mtdinfo.c
> @@ -239,8 +239,14 @@ static void print_region_info(const struct mtd_dev_info *mtd)
>  	region_info_t reginfo;
>  	int r, fd;
>  
> -	/* If we don't have any region info, just return */
> -	if (!args.map && mtd->region_cnt == 0)
> +	/*
> +	 * If we don't have any region info, just return
> +	 *
> +	 * FIXME: We can't get region_info (via ioctl) without having the MTD
> +	 *        node path. This is a problem for `mtdinfo -a', for example,
> +	 *        since it doesn't provide any filepath information.
> +	 */
> +	if (!args.node || !args.map && mtd->region_cnt == 0)

 I suggest adding some round brackets:

	if (!args.node || (!args.map && mtd->region_cnt == 0))

 Less confusing.  Otherwise, looks Ok
 to me (not actually built or tested).
cheers!
	-blf-

>  		return;
>  
>  	/* First open the device so we can query it */
> -- 
> 1.7.0.4

-- 
Brian FOSTER
Principal MTS, Software
Maxim Integrated Products (Microcontroller BU), formerly Innova Card
Web    : http://www.maxim-ic.com/

Re: [PATCH] mtdinfo: don't open NULL pointer when getting region_info with `-a'

[Brian Norris]

Hi (other) Brian,

On Fri, Aug 5, 2011 at 12:29 AM, Brian Foster <brian.foster@maxim-ic.com> wrote:
>  I suggest adding some round brackets:
>
>        if (!args.node || (!args.map && mtd->region_cnt == 0))
>
>  Less confusing.  Otherwise, looks Ok
>  to me (not actually built or tested).

Thanks for the comment. But I'm guessing this patch isn't the best
idea after all (unless Mike's plan doesn't work).

Anyway, it'd probably help if you test the final product, since you're
the one who actually sees the error!

Thanks,
Brian