| Message ID | 20230609061619.57756-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | thunderbolt: handle possible NULL pointer from get_device() | expand |
On Fri, Jun 09, 2023 at 09:16:19AM +0300, Dmitry Antipov wrote: > Handle possible NULL pointer returned by 'get_device()' > in 'tb_xdomain_alloc()' and 'remove_unplugged_switch()'. Sorry, but how can that happen? > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > drivers/thunderbolt/icm.c | 5 +++++ > drivers/thunderbolt/xdomain.c | 4 ++++ > 2 files changed, 9 insertions(+) > > diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c > index 86521ebb2579..40ab6104a437 100644 > --- a/drivers/thunderbolt/icm.c > +++ b/drivers/thunderbolt/icm.c > @@ -2035,6 +2035,11 @@ static void remove_unplugged_switch(struct tb_switch *sw) > { > struct device *parent = get_device(sw->dev.parent); > > + if (!parent) { This will never fail, how did you test this? > + tb_warn(sw->tb, "no parent of switch %pUb\n", sw->uuid); > + return; > + } > + > pm_runtime_get_sync(parent); > > /* > diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c > index e2b54887d331..a0ee683d752e 100644 > --- a/drivers/thunderbolt/xdomain.c > +++ b/drivers/thunderbolt/xdomain.c > @@ -1883,6 +1883,8 @@ struct tb_xdomain *tb_xdomain_alloc(struct tb *tb, struct device *parent, > > device_initialize(&xd->dev); > xd->dev.parent = get_device(parent); > + if (!xd->dev.parent) Again, how did you test this? How can that ever happen? thanks, greg k-h
On 6/9/23 09:30, Greg KH wrote:
> Sorry, but how can that happen?
Hmmm.. there should be a complete mess in the device tree (e.g. passing
root device with no parent where the regular device is expected). If
you're sure that this doesn't worth checking then please disregard this.
Dmitry
On Fri, Jun 09, 2023 at 10:15:36AM +0300, Dmitry Antipov wrote: > On 6/9/23 09:30, Greg KH wrote: > > > Sorry, but how can that happen? > > Hmmm.. there should be a complete mess in the device tree (e.g. passing > root device with no parent where the regular device is expected). If > you're sure that this doesn't worth checking then please disregard this. Again, how did you test this? And yes, the driver model guarantees that a child can never have a NULL parent (except for the root device, but that's a different story...) thanks, greg k-h
On 6/9/23 10:46, Greg KH wrote:
> Again, how did you test this?
Did you look at the patch header? For that particular case, the static
analysis tool complains that the value returned by get_device() is most
likely should be checked just because it is checked on a lot of other
code paths. Usually it may be a good precaution to handle the very rare
and unexpected errors; again, if you're sure that this is not the case,
just disregard it.
Dmitry
On Fri, Jun 09, 2023 at 11:05:59AM +0300, Dmitry Antipov wrote: > On 6/9/23 10:46, Greg KH wrote: > > > Again, how did you test this? > > Did you look at the patch header? For that particular case, the static > analysis tool complains that the value returned by get_device() is most > likely should be checked just because it is checked on a lot of other > code paths. Usually it may be a good precaution to handle the very rare > and unexpected errors; again, if you're sure that this is not the case, > just disregard it. Just because a static tool said "this might be wrong" does not mean you do not need to actually test your change or do some work to verify that it is a sane change at all. So far I have seen more and more false-positives from this "tool" of your group that I am very inclined to just tell all kernel maintainers to ignore them for a very long time as you are not following the documented rules for such patches as outlined in Documentation/process/researcher-guidelines.rst Please read that and fix your tool, and your submission process, I've said this many times already. greg k-h
On 09.06.2023 11:05, Dmitry Antipov wrote: > On 6/9/23 10:46, Greg KH wrote: > >> Again, how did you test this? > > Did you look at the patch header? For that particular case, the static > analysis tool complains that the value returned by get_device() is most > likely should be checked just because it is checked on a lot of other > code paths. Usually it may be a good precaution to handle the very rare > and unexpected errors; again, if you're sure that this is not the case, > just disregard it. Nevertheless it is your responsibility to check if such situation can happen in practice in this particular place. And if there are non-obvious assumptions that prevent you to make such analysis before sending a patch, you are welcome to prepare a patch that improves documentation. Best regards, Alexey
On 6/9/23 11:20, Greg KH wrote: > So far I have seen more and more false-positives from this "tool" of > your group that I am very inclined to just tell all kernel maintainers > to ignore them for a very long time as you are not following the > documented rules for such patches as outlined in > Documentation/process/researcher-guidelines.rst > > Please read that and fix your tool, and your submission process, I've > said this many times already. There might be a lot of definitions of what "research" actually is. I realize that the maintainers may be very busy, but is it completely illegal to sent a patch just to raise the flag and ask to share an expertise? Note this was not a private e-mail, and it looks a bit strange to treat it just like an attempt to waste your personal time. And don't you think that such a policy definitely cuts off the beginners and makes the community less friendly? Dmitry
On Fri, Jun 09, 2023 at 12:19:02PM +0300, Dmitry Antipov wrote: > On 6/9/23 11:20, Greg KH wrote: > > > So far I have seen more and more false-positives from this "tool" of > > your group that I am very inclined to just tell all kernel maintainers > > to ignore them for a very long time as you are not following the > > documented rules for such patches as outlined in > > Documentation/process/researcher-guidelines.rst > > > > Please read that and fix your tool, and your submission process, I've > > said this many times already. > > There might be a lot of definitions of what "research" actually is. > I realize that the maintainers may be very busy, but is it completely > illegal to sent a patch just to raise the flag and ask to share an > expertise? Note this was not a private e-mail, and it looks a bit > strange to treat it just like an attempt to waste your personal time. As a maintainer, you are asking for others to check your work. If you are sending stuff that is incorrect, yes, that is wasting their time. > And don't you think that such a policy definitely cuts off the > beginners and makes the community less friendly? Again, you are asking me to verify that your random tool is somehow actually sending out correct messages by asking me to review a patch for it? It's your responsibility to verify that your tool works properly, as you are in control of it, so yes, you MUST follow those research guidelines as that is EXACTLY what they were written for due to problems exactly like this. I have pointed out how broken this "tool" is numerous times in the past, this should not come as a surprise to you at all. greg k-h
diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c index 86521ebb2579..40ab6104a437 100644 --- a/drivers/thunderbolt/icm.c +++ b/drivers/thunderbolt/icm.c @@ -2035,6 +2035,11 @@ static void remove_unplugged_switch(struct tb_switch *sw) { struct device *parent = get_device(sw->dev.parent); + if (!parent) { + tb_warn(sw->tb, "no parent of switch %pUb\n", sw->uuid); + return; + } + pm_runtime_get_sync(parent); /* diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c index e2b54887d331..a0ee683d752e 100644 --- a/drivers/thunderbolt/xdomain.c +++ b/drivers/thunderbolt/xdomain.c @@ -1883,6 +1883,8 @@ struct tb_xdomain *tb_xdomain_alloc(struct tb *tb, struct device *parent, device_initialize(&xd->dev); xd->dev.parent = get_device(parent); + if (!xd->dev.parent) + goto err_free_remote_uuid; xd->dev.bus = &tb_bus_type; xd->dev.type = &tb_xdomain_type; xd->dev.groups = xdomain_attr_groups; @@ -1902,6 +1904,8 @@ struct tb_xdomain *tb_xdomain_alloc(struct tb *tb, struct device *parent, return xd; +err_free_remote_uuid: + kfree(xd->remote_uuid); err_free_local_uuid: kfree(xd->local_uuid); err_free:
Handle possible NULL pointer returned by 'get_device()' in 'tb_xdomain_alloc()' and 'remove_unplugged_switch()'. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- drivers/thunderbolt/icm.c | 5 +++++ drivers/thunderbolt/xdomain.c | 4 ++++ 2 files changed, 9 insertions(+)