Commit aba0e909 authored by Moshe Shemesh's avatar Moshe Shemesh Committed by Jakub Kicinski
Browse files

devlink: Hold devlink lock on health reporter dump get 



Devlink health dump get callback should take devlink lock as any other
devlink callback. Otherwise, since devlink_mutex was removed, this
callback is not protected from a race of the reporter being destroyed
while handling the callback.

Add devlink lock to the callback and to any call for
devlink_health_do_dump(). This should be safe as non of the drivers dump
callback implementation takes devlink lock.

As devlink lock is added to any callback of dump, the reporter dump_lock
is now redundant and can be removed.

Fixes: d3efc2a6 ("net: devlink: remove devlink_mutex")
Signed-off-by: default avatarMoshe Shemesh <moshe@nvidia.com>
Reviewed-by: default avatarJiri Pirko <jiri@nvidia.com>
Reviewed-by: default avatarPrzemek Kitszel <przemyslaw.kitszel@intel.com>
Link: https://lore.kernel.org/r/1696510216-189379-1-git-send-email-moshe@nvidia.com


Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parent c4d49196

net/devlink/health.c

+16 −14
Original line number Diff line number Diff line
@@ -58,7 +58,6 @@ struct devlink_health_reporter { 
	struct devlink *devlink;

	struct devlink_port *devlink_port;

	struct devlink_fmsg *dump_fmsg;

	struct mutex dump_lock; /* lock parallel read/write from dump buffers */

	u64 graceful_period;

	bool auto_recover;

	bool auto_dump;
@@ -125,7 +124,6 @@ __devlink_health_reporter_create(struct devlink *devlink, 
	reporter->graceful_period = graceful_period;

	reporter->auto_recover = !!ops->recover;

	reporter->auto_dump = !!ops->dump;

	mutex_init(&reporter->dump_lock);

	return reporter;

}
@@ -226,7 +224,6 @@ EXPORT_SYMBOL_GPL(devlink_health_reporter_create); 
static void

devlink_health_reporter_free(struct devlink_health_reporter *reporter)

{

	mutex_destroy(&reporter->dump_lock);

	if (reporter->dump_fmsg)

		devlink_fmsg_free(reporter->dump_fmsg);

	kfree(reporter);
@@ -625,10 +622,10 @@ int devlink_health_report(struct devlink_health_reporter *reporter, 
	}



	if (reporter->auto_dump) {

		mutex_lock(&reporter->dump_lock);

		devl_lock(devlink);

		/* store current dump of current error, for later analysis */

		devlink_health_do_dump(reporter, priv_ctx, NULL);

		mutex_unlock(&reporter->dump_lock);

		devl_unlock(devlink);

	}



	if (!reporter->auto_recover)
@@ -1262,7 +1259,7 @@ int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb, 
}



static struct devlink_health_reporter *

devlink_health_reporter_get_from_cb(struct netlink_callback *cb)

devlink_health_reporter_get_from_cb_lock(struct netlink_callback *cb)

{

	const struct genl_info *info = genl_info_dump(cb);

	struct devlink_health_reporter *reporter;
@@ -1272,10 +1269,12 @@ devlink_health_reporter_get_from_cb(struct netlink_callback *cb) 
	devlink = devlink_get_from_attrs_lock(sock_net(cb->skb->sk), attrs);

	if (IS_ERR(devlink))

		return NULL;

	devl_unlock(devlink);



	reporter = devlink_health_reporter_get_from_attrs(devlink, attrs);

	if (!reporter) {

		devl_unlock(devlink);

		devlink_put(devlink);

	}

	return reporter;

}
@@ -1284,16 +1283,20 @@ int devlink_nl_cmd_health_reporter_dump_get_dumpit(struct sk_buff *skb, 
{

	struct devlink_nl_dump_state *state = devlink_dump_state(cb);

	struct devlink_health_reporter *reporter;

	struct devlink *devlink;

	int err;



	reporter = devlink_health_reporter_get_from_cb(cb);

	reporter = devlink_health_reporter_get_from_cb_lock(cb);

	if (!reporter)

		return -EINVAL;



	if (!reporter->ops->dump)

	devlink = reporter->devlink;

	if (!reporter->ops->dump) {

		devl_unlock(devlink);

		devlink_put(devlink);

		return -EOPNOTSUPP;

	}



	mutex_lock(&reporter->dump_lock);

	if (!state->idx) {

		err = devlink_health_do_dump(reporter, NULL, cb->extack);

		if (err)
@@ -1309,7 +1312,8 @@ int devlink_nl_cmd_health_reporter_dump_get_dumpit(struct sk_buff *skb, 
	err = devlink_fmsg_dumpit(reporter->dump_fmsg, skb, cb,

				  DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET);

unlock:

	mutex_unlock(&reporter->dump_lock);

	devl_unlock(devlink);

	devlink_put(devlink);

	return err;

}
@@ -1326,9 +1330,7 @@ int devlink_nl_cmd_health_reporter_dump_clear_doit(struct sk_buff *skb, 
	if (!reporter->ops->dump)

		return -EOPNOTSUPP;



	mutex_lock(&reporter->dump_lock);

	devlink_health_dump_clear(reporter);

	mutex_unlock(&reporter->dump_lock);

	return 0;

}