Message ID | 20160715110629.GB9258@mwanda |
---|---|
State | Accepted |
Commit | dc01a28d80a42cef08c94dfc595565aaebe46d15 |
Headers | show |
+ 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
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
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
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
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
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;
We check for NULL but then dereference "info->mtd" on the next line. Fixes: 72169755cf36 ('mtd: maps: sa1100-flash: show parent device in sysfs') Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>