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?
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
