Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference

Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference

[Brian Norris]

+ stable

Hi Dan,

Patch looks good, but one question.

On Fri, Jul 15, 2016 at 02:06:30PM +0300, Dan Carpenter wrote:
> We check for NULL but then dereference "info->mtd" on the next line.
> 
> Fixes: 72169755cf36 ('mtd: maps: sa1100-flash: show parent device in sysfs')

What am I supposed to do about tags like this? It appears that the
-stable folks have started taking patches with a 'Fixes' tag alone [0],
even though that's not mentioned in [1]. I ask because I strongly
suspect this patch doesn't fit the rules in [1] -- it quite likely has
only been compile tested; and it qualifies quite well as violating
bullet 4:

"""
 - It must fix a real bug that bothers people (not a, "This could be a
   problem..." type thing).
"""

So, I'd like to keep the tag, but I'd like to avoid having to NAK it in
the stable review process. (And really, I often don't care enough to
even do that. I believe there's a very low chance that something like
this would cause additional problems more than the original bug.)

Regards,
Brian

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/mtd/maps/sa1100-flash.c b/drivers/mtd/maps/sa1100-flash.c
> index 142fc3d..784c6e1 100644
> --- a/drivers/mtd/maps/sa1100-flash.c
> +++ b/drivers/mtd/maps/sa1100-flash.c
> @@ -230,8 +230,10 @@ static struct sa_info *sa1100_setup_mtd(struct platform_device *pdev,
>  
>  		info->mtd = mtd_concat_create(cdev, info->num_subdev,
>  					      plat->name);
> -		if (info->mtd == NULL)
> +		if (info->mtd == NULL) {
>  			ret = -ENXIO;
> +			goto err;
> +		}
>  	}
>  	info->mtd->dev.parent = &pdev->dev;
>  

[0] I haven't tried to prove that all patches with 'Fixes' tags go to
the -stable queue, but I know at least that this commit:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3b5394a3ccffbfa1d1d448d48742853a862822c4

ended up in v4.5.y here:

https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=800a0b8a973b4262c92c228043cd17455cdf1a15

and IIRC, there are plenty more like that.

[1] Documentation/stable_kernel_rules.txt

Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference

[Greg Kroah-Hartman]

On Fri, Jul 15, 2016 at 05:32:09PM -0700, Brian Norris wrote:
> + stable
> 
> Hi Dan,
> 
> Patch looks good, but one question.
> 
> On Fri, Jul 15, 2016 at 02:06:30PM +0300, Dan Carpenter wrote:
> > We check for NULL but then dereference "info->mtd" on the next line.
> > 
> > Fixes: 72169755cf36 ('mtd: maps: sa1100-flash: show parent device in sysfs')
> 
> What am I supposed to do about tags like this? It appears that the
> -stable folks have started taking patches with a 'Fixes' tag alone [0],
> even though that's not mentioned in [1]. I ask because I strongly
> suspect this patch doesn't fit the rules in [1] -- it quite likely has
> only been compile tested; and it qualifies quite well as violating
> bullet 4:
> 
> """
>  - It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing).
> """
> 
> So, I'd like to keep the tag, but I'd like to avoid having to NAK it in
> the stable review process. (And really, I often don't care enough to
> even do that. I believe there's a very low chance that something like
> this would cause additional problems more than the original bug.)

Only sometimes will I pick up something that only has a fixes: tag in
it, not all the time, I try to review the patch to see if it does match
the rules or not.

But, fixing an oops is a good thing, I'm sure you can figure out how to
trigger it otherwise you would not be taking such a patch as it would be
not be needed :)

thanks,

greg k-h

Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference

[Brian Norris]

Hi,

On Sat, Jul 16, 2016 at 09:48:25AM +0900, Greg Kroah-Hartman wrote:
> On Fri, Jul 15, 2016 at 05:32:09PM -0700, Brian Norris wrote:
> > + stable
> > 
> > Hi Dan,
> > 
> > Patch looks good, but one question.
> > 
> > On Fri, Jul 15, 2016 at 02:06:30PM +0300, Dan Carpenter wrote:
> > > We check for NULL but then dereference "info->mtd" on the next line.
> > > 
> > > Fixes: 72169755cf36 ('mtd: maps: sa1100-flash: show parent device in sysfs')
> > 
> > What am I supposed to do about tags like this? It appears that the
> > -stable folks have started taking patches with a 'Fixes' tag alone [0],
> > even though that's not mentioned in [1]. I ask because I strongly
> > suspect this patch doesn't fit the rules in [1] -- it quite likely has
> > only been compile tested; and it qualifies quite well as violating
> > bullet 4:
> > 
> > """
> >  - It must fix a real bug that bothers people (not a, "This could be a
> >    problem..." type thing).
> > """
> > 
> > So, I'd like to keep the tag, but I'd like to avoid having to NAK it in
> > the stable review process. (And really, I often don't care enough to
> > even do that. I believe there's a very low chance that something like
> > this would cause additional problems more than the original bug.)
> 
> Only sometimes will I pick up something that only has a fixes: tag in
> it, not all the time, I try to review the patch to see if it does match
> the rules or not.

OK, good to know. I've seen other -stable maintainers do similarly, but
I don't know what their process is.

> But, fixing an oops is a good thing, I'm sure you can figure out how to
> trigger it otherwise you would not be taking such a patch as it would be
> not be needed :)

Of course. But it's still not always clear whether such fixes will
trigger other errors in poorly-tested error paths. Is (for instance) an
oops that we know about better than a use-after-free that we don't know
about?

Anyway, applied to l2-mtd.git.

Regards,
Brian

Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference

[Dan Carpenter]

I like the Fixes tag because it was my invention.  :)  It's a separate
thing from -stable.

It's nice for reviewing so you can see the original intent of the patch
you're fixing.  Also it forces you to find the original authors and CC
them so hopefully they Ack the patch.  The other thing is it lets you
collect data about which patches introduce bugs and how quickly they
get fixed.  So for example, lwn.net recently had an article about bug
that are backported into the -stable tree.

regards,
dan carpenter

Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference

[Brian Norris]

On Sat, Jul 16, 2016 at 12:00:41PM +0300, Dan Carpenter wrote:
> I like the Fixes tag because it was my invention.  :)  It's a separate
> thing from -stable.

Ha, nice. Well I have nothing against the tag, and nothing against this
patch. It's good to know that the Fixes tag is not (necessarily) a
request-for-stable tag.

> It's nice for reviewing so you can see the original intent of the patch
> you're fixing.  Also it forces you to find the original authors and CC
> them so hopefully they Ack the patch.  The other thing is it lets you
> collect data about which patches introduce bugs and how quickly they
> get fixed.  So for example, lwn.net recently had an article about bug
> that are backported into the -stable tree.

All good things. I know personally it's helpful when tracking down bugs,
or backporting drivers or features.

Regards,
Brian