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