[bug report] staging: qlge: Initialize devlink health dump framework

[Coiby Xu]

On Wed, Feb 03, 2021 at 05:36:01PM +0300, Dan Carpenter wrote:
>On Wed, Feb 03, 2021 at 09:45:51PM +0800, Coiby Xu wrote:
>> Hi Dan,
>>
>>
>> On Wed, Feb 03, 2021 at 12:42:50PM +0300, Dan Carpenter wrote:
>> > Hello Coiby Xu,
>> >
>> > The patch 953b94009377: "staging: qlge: Initialize devlink health
>> > dump framework" from Jan 23, 2021, leads to the following static
>> > checker warning:
>> >
>> > 	drivers/staging/qlge/qlge_devlink.c:163 qlge_health_create_reporters()
>> > 	error: uninitialized symbol 'reporter'.
>> >
>> > drivers/staging/qlge/qlge_devlink.c
>> >   151  void qlge_health_create_reporters(struct qlge_adapter *priv)
>> >   152  {
>> >   153          struct devlink_health_reporter *reporter;
>> >   154          struct devlink *devlink;
>> >   155
>> >   156          devlink = priv_to_devlink(priv);
>> >   157          priv->reporter =
>> >   158                  devlink_health_reporter_create(devlink, &qlge_reporter_ops,
>> >   159                                                 0, priv);
>> >   160          if (IS_ERR(priv->reporter))
>> >   161                  netdev_warn(priv->ndev,
>> >   162                              "Failed to create reporter, err = %ld\n",
>> >   163                              PTR_ERR(reporter));
>> >
>> > Obviously the static checker is correct because we initialized
>> > "priv->reporter" instead of "reporter".
>> >
>> > It's not clear to me how "reporter" is used.  Presumably this should be:
>> >
>> > 	reporter = devlink_health_reporter_create(devlink, &qlge_reporter_ops,
>> > 						  0, priv);
>> > 	if (IS_ERR(reporter)) {
>> > 		netdev_warn(priv->ndev,
>> > 			    "Failed to create reporter, err = %ld\n",
>> > 			    PTR_ERR(reporter));
>> > 		return;
>> > 	}
>> > 	priv->reporter = reporter;
>> >
>>
>> Thank you for finding this issue! "struct devlink_health_reporter
>> *reporter" is not needed since priv->reporter is used instead which
>> I think simplifies the code.
>>
>> > But I can't actually find where "priv->reporter" is checked against
>> > NULL.  There should be some NULL checks, right?
>> >
>>
>> There is no need to do NULL check since devlink_health_reporter_create
>> has done the job for us,
>>
>> // net/core/devlink.c
>> __devlink_health_reporter_create(struct devlink *devlink,
>> 				 const struct devlink_health_reporter_ops *ops,
>> 				 u64 graceful_period, void *priv)
>> {
>> 	reporter = kzalloc(sizeof(*reporter), GFP_KERNEL);
>> 	if (!reporter)
>> 		return ERR_PTR(-ENOMEM);
>>
>> }
>
>No, Sorry I was unclear.  I mean that instead of error handling this
>qlge_health_create_reporters() function just prints an error:
>
> 		netdev_warn(priv->ndev,
> 			    "Failed to create reporter, err = %ld\n",
> 			    PTR_ERR(priv->reporter));
>
>Then it looks like it gets passed to qlge_reporter_coredump() which
>dereferences "reporter" without checking.  That will crash, right?
>

Now I see what you mean. Thanks for the explanation! I'll send a patch
to address this issue.

>regards,
>dan carpenter
>

--
Best regards,
Coiby