* [patch] mtd: maps: sa1100-flash: potential NULL dereference @ 2016-07-15 11:06 ` Dan Carpenter 0 siblings, 0 replies; 12+ messages in thread From: Dan Carpenter @ 2016-07-15 11:06 UTC (permalink / raw) To: David Woodhouse, Frans Klaver; +Cc: Brian Norris, linux-mtd, kernel-janitors 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> 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; ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [patch] mtd: maps: sa1100-flash: potential NULL dereference @ 2016-07-15 11:06 ` Dan Carpenter 0 siblings, 0 replies; 12+ messages in thread From: Dan Carpenter @ 2016-07-15 11:06 UTC (permalink / raw) To: David Woodhouse, Frans Klaver; +Cc: Brian Norris, linux-mtd, kernel-janitors 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> 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; ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference 2016-07-15 11:06 ` Dan Carpenter @ 2016-07-16 0:32 ` Brian Norris -1 siblings, 0 replies; 12+ messages in thread From: Brian Norris @ 2016-07-16 0:32 UTC (permalink / raw) To: Dan Carpenter Cc: David Woodhouse, Frans Klaver, linux-mtd, kernel-janitors, Greg Kroah-Hartman, stable, linux-kernel + 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference @ 2016-07-16 0:32 ` Brian Norris 0 siblings, 0 replies; 12+ messages in thread From: Brian Norris @ 2016-07-16 0:32 UTC (permalink / raw) To: Dan Carpenter Cc: David Woodhouse, Frans Klaver, linux-mtd, kernel-janitors, Greg Kroah-Hartman, stable, linux-kernel + 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;5394a3ccffbfa1d1d448d48742853a862822c4 ended up in v4.5.y here: https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id€0a0b8a973b4262c92c228043cd17455cdf1a15 and IIRC, there are plenty more like that. [1] Documentation/stable_kernel_rules.txt -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference 2016-07-16 0:32 ` Brian Norris @ 2016-07-16 0:48 ` Greg Kroah-Hartman -1 siblings, 0 replies; 12+ messages in thread From: Greg Kroah-Hartman @ 2016-07-16 0:48 UTC (permalink / raw) To: Brian Norris Cc: Dan Carpenter, David Woodhouse, Frans Klaver, linux-mtd, kernel-janitors, stable, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference @ 2016-07-16 0:48 ` Greg Kroah-Hartman 0 siblings, 0 replies; 12+ messages in thread From: Greg Kroah-Hartman @ 2016-07-16 0:48 UTC (permalink / raw) To: Brian Norris Cc: Dan Carpenter, David Woodhouse, Frans Klaver, linux-mtd, kernel-janitors, stable, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference 2016-07-16 0:48 ` Greg Kroah-Hartman @ 2016-07-16 1:46 ` Brian Norris -1 siblings, 0 replies; 12+ messages in thread From: Brian Norris @ 2016-07-16 1:46 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Dan Carpenter, David Woodhouse, Frans Klaver, linux-mtd, kernel-janitors, stable, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference @ 2016-07-16 1:46 ` Brian Norris 0 siblings, 0 replies; 12+ messages in thread From: Brian Norris @ 2016-07-16 1:46 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Dan Carpenter, David Woodhouse, Frans Klaver, linux-mtd, kernel-janitors, stable, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference 2016-07-16 0:32 ` Brian Norris @ 2016-07-16 9:00 ` Dan Carpenter -1 siblings, 0 replies; 12+ messages in thread From: Dan Carpenter @ 2016-07-16 9:00 UTC (permalink / raw) To: Brian Norris Cc: David Woodhouse, Frans Klaver, linux-mtd, kernel-janitors, Greg Kroah-Hartman, stable, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference @ 2016-07-16 9:00 ` Dan Carpenter 0 siblings, 0 replies; 12+ messages in thread From: Dan Carpenter @ 2016-07-16 9:00 UTC (permalink / raw) To: Brian Norris Cc: David Woodhouse, Frans Klaver, linux-mtd, kernel-janitors, Greg Kroah-Hartman, stable, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference 2016-07-16 9:00 ` Dan Carpenter @ 2016-07-17 3:54 ` Brian Norris -1 siblings, 0 replies; 12+ messages in thread From: Brian Norris @ 2016-07-17 3:54 UTC (permalink / raw) To: Dan Carpenter Cc: David Woodhouse, Frans Klaver, linux-mtd, kernel-janitors, Greg Kroah-Hartman, stable, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mtd: maps: sa1100-flash: potential NULL dereference @ 2016-07-17 3:54 ` Brian Norris 0 siblings, 0 replies; 12+ messages in thread From: Brian Norris @ 2016-07-17 3:54 UTC (permalink / raw) To: Dan Carpenter Cc: David Woodhouse, Frans Klaver, linux-mtd, kernel-janitors, Greg Kroah-Hartman, stable, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-07-17 3:54 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-15 11:06 [patch] mtd: maps: sa1100-flash: potential NULL dereference Dan Carpenter 2016-07-15 11:06 ` Dan Carpenter 2016-07-16 0:32 ` Brian Norris 2016-07-16 0:32 ` Brian Norris 2016-07-16 0:48 ` Greg Kroah-Hartman 2016-07-16 0:48 ` Greg Kroah-Hartman 2016-07-16 1:46 ` Brian Norris 2016-07-16 1:46 ` Brian Norris 2016-07-16 9:00 ` Dan Carpenter 2016-07-16 9:00 ` Dan Carpenter 2016-07-17 3:54 ` Brian Norris 2016-07-17 3:54 ` Brian Norris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.