Hey Alex, Alex Elder <elder@xxxxxxxx> writes: > On 3/25/24 1:50 PM, Greg Kroah-Hartman wrote: >> On Mon, Mar 25, 2024 at 01:31:34PM -0500, Alex Elder wrote: >>> On 3/25/24 12:25 PM, Greg Kroah-Hartman wrote: >>>> On Thu, Mar 07, 2024 at 09:48:13AM +0000, Rui Miguel Silva wrote: >>>>> If channel for the given node is not found we return null from >>>>> get_channel_from_mode. Make sure we validate the return pointer >>>>> before using it in two of the missing places. >>>>> >>>>> This was originally reported in [0]: >>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>>>> >>>>> [0] https://lore.kernel.org/all/20240301190425.120605-1-m.lobanov@xxxxxxxxxxxx >>>>> >>>>> Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") >>>>> Reported-by: Mikhail Lobanov <m.lobanov@xxxxxxxxxxxx> >>>>> Suggested-by: Mikhail Lobanov <m.lobanov@xxxxxxxxxxxx> >>>>> Suggested-by: Alex Elder <elder@xxxxxxxx> >>>>> Signed-off-by: Rui Miguel Silva <rmfrfs@xxxxxxxxx> >>>>> --- >>>>> drivers/staging/greybus/light.c | 6 +++++- >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c >>>>> index c6bd86a5335a..6f10b9e2c053 100644 >>>>> --- a/drivers/staging/greybus/light.c >>>>> +++ b/drivers/staging/greybus/light.c >>>>> @@ -147,6 +147,9 @@ static int __gb_lights_flash_brightness_set(struct gb_channel *channel) >>>>> channel = get_channel_from_mode(channel->light, >>>>> GB_CHANNEL_MODE_TORCH); >>>>> + if (!channel) >>>>> + return -EINVAL; >>>>> + >>>>> /* For not flash we need to convert brightness to intensity */ >>>>> intensity = channel->intensity_uA.min + >>>>> (channel->intensity_uA.step * channel->led->brightness); >>>>> @@ -549,7 +552,8 @@ static int gb_lights_light_v4l2_register(struct gb_light *light) >>>>> } >>>>> channel_flash = get_channel_from_mode(light, GB_CHANNEL_MODE_FLASH); >>>>> - WARN_ON(!channel_flash); >>>>> + if (WARN_ON(!channel_flash)) >>>>> + return -EINVAL; >>>> >>>> We should NOT crash machines just because of this, the WARN_ON() should >>>> be removed and just properly handle the error please. >>> >>> Greg, WARN_ON() doesn't normally crash the machine. That said, >>> it's reasonable to remove the WARN_ON(). >> >> The huge majority of running Linux systems in the world run with >> panic-on-warn enabled, including the one in your pocket :( > > I did not know that. Then WARN_ON() is no better than BUG_ON(). > I'm still learning. Thank you. I also lost track of all this failure cascade options that normally take the all system down. Thanks anyway for the comments, Cheers, Rui > > -Alex > >>> I think the purpose of the warning is that this is a case that >>> should "never happen," so if it does, it's making some noise. >> >> Making noise by rebooting the box is not good. >> >> thanks, >> >> greg k-h
