Re: [PATCH 20/23] net: hpe: ss1: add the CXI core interfaces

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [High] net: hpe: ss1: unsafe configfs_unregister_group() on mkdir groups in teardown
- [High] net: hpe: ss1: invalid mutex_lock_nested() on singleton dev_list_mutex
- [Medium] net: hpe: ss1: cdev set after configfs_register_group() in device init
- [Medium] net: hpe: ss1: kfree() instead of config_item_put() leaks ci_name on error
- [Medium] net: hpe: ss1: uninitialized uid, gid, and netns passed in AC type_store()
- [Medium] net: hpe: ss1: configfs AC entry state desync, leaks, and wrong deletion
- [Medium] net: hpe: ss1: prefix strncmp() instead of strcmp() in configfs make_group()
- [Medium] net: hpe: ss1: signed limit parsing and %ld format for size_t in cxi_config.c
- [Medium] net: hpe: ss1: premature match mutation in tx/rx_prof_attr_match_store()
- [Medium] net: hpe: ss1: unvalidated runtime sysfs writes to tg_threshold parameter
- [Medium] net: hpe: ss1: system_type_identifier validated after hw_register() in cxi_init()
- [Medium] net: hpe: ss1: inverted boolean polarity in cxi_pml_recovery_set()
- [Low] net: hpe: ss1: undocumented module parameters in cxi_core.c
- [Low] net: hpe: ss1: stale TC state in cxi_cfg_tx_prof_tc show callbacks
--

commit a96b9ed904f5550cc657971c4b284154997f9863
Author: Doug Ledford <doug.ledford@hpe.com>

net: hpe: ss1: add the CXI core interfaces

This commit adds the CXI core interface, including client registration,
the RDMA auxiliary device, link control callbacks, and configfs support
for resource groups and RX/TX profiles.

> diff --git a/drivers/net/ethernet/hpe/ss1/cxi_config.c b/drivers/net/ethernet/hpe/ss1/cxi_config.c
> new file mode 100644
> index 0000000000000..dd604753daeeb
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/ss1/cxi_config.c
[ ... ]
> +static ssize_t cxi_cfg_rgroup_ac_entry_sub_type_store(struct config_item *item,
> +						      const char *page,
> +						      size_t count)
> +{
> +	struct cxi_cfg_rgroup_ac_entry_sub *ac_entry_sub;
> +	int rc;
> +	enum cxi_ac_type ac_type_val;
> +	union cxi_ac_data ac_data_t = {};
> +	unsigned int ac_entry_id;
> +
> +	ac_entry_sub = to_cxi_cfg_rgroup_ac_entry_sub(item);
> +	if (!ac_entry_sub->rgroup) {
> +		pr_debug("Unable to get rgroup reference\n");
> +		return -EINVAL;
> +	}
> +
> +	rc = kstrtouint(page, 0, (unsigned int *)&ac_type_val);
> +	if (rc)
> +		return rc;
> +
> +	if (ac_type_val == CXI_AC_UID)
> +		ac_data_t.uid = ac_entry_sub->ac_data.uid;
> +	else if (ac_type_val == CXI_AC_GID)
> +		ac_data_t.gid = ac_entry_sub->ac_data.gid;
> +	else if (ac_type_val == CXI_AC_NETNS)
> +		ac_data_t.netns = ac_entry_sub->ac_data.netns;
> +	else if (ac_type_val == CXI_AC_OPEN)
> +		pr_debug("Setting type to CXI_AC_OPEN\n");
> +	else
> +		return -EINVAL;
> +
> +	rc = cxi_rgroup_add_ac_entry(ac_entry_sub->rgroup,
> +				     ac_type_val,
> +				     &ac_data_t,
> +				     &ac_entry_id);
> +	if (rc < 0) {
> +		pr_debug("Unable to add ac entry to resource group: err val %d\n", rc);
> +		return rc;
> +	}
> +
> +	ac_entry_sub->ac_type = ac_type_val;
> +	ac_entry_sub->id = ac_entry_id;
> +
> +	return count;
> +}
[ ... ]
> +static ssize_t cxi_cfg_rgroup_ac_entry_sub_data_store(struct config_item *item,
> +						      const char *page,
> +						      size_t count)
> +{
> +	struct cxi_cfg_rgroup_ac_entry_sub *ac_entry_sub;
> +	int rc;
> +	unsigned int ac_data_val;
> +
> +	ac_entry_sub = to_cxi_cfg_rgroup_ac_entry_sub(item);
> +
> +	rc = kstrtouint(page, 0, &ac_data_val);
> +	if (rc)
> +		return rc;
> +
> +	if (ac_entry_sub->ac_type == CXI_AC_UID) {
> +		ac_entry_sub->ac_data.uid = ac_data_val;
> +		return count;
> +	} else if (ac_entry_sub->ac_type == CXI_AC_GID) {
> +		ac_entry_sub->ac_data.gid = ac_data_val;
> +		return count;
> +	} else if (ac_entry_sub->ac_type == CXI_AC_NETNS) {
> +		ac_entry_sub->ac_data.netns = ac_data_val;
> +		return count;
> +	}
> +
> +	return -EINVAL;
> +}
[ ... ]
> +static struct
> +config_group *cxi_cfg_rgroup_ac_entry_make_group(struct config_group *group,
> +						 const char *name)
> +{
[ ... ]
> +	ac_entry_sub->rgroup = ac_entry->rgroup;
> +	ac_entry_sub->ac_type = CXI_AC_UID;
> +	ac_entry_sub->ac_data.uid = 0;
> +
> +	return &ac_entry_sub->group;
> +}
> +
> +static void
> +cxi_cfg_rgroup_ac_entry_drop_item(struct config_group *group,
> +				  struct config_item *item)
> +{
> +	struct cxi_cfg_rgroup_ac_entry_sub *ac_entry_sub;
> +	unsigned int ac_entry_id;
> +	int rc;
> +
> +	ac_entry_sub = container_of(item, struct cxi_cfg_rgroup_ac_entry_sub, group.cg_item);
> +
> +	if (ac_entry_sub->ac_type == CXI_AC_UID) {
> +		rc = cxi_rgroup_get_ac_entry_id_by_data(ac_entry_sub->rgroup,
> +							CXI_AC_UID,
> +							&ac_entry_sub->ac_data,
> +							&ac_entry_id);
> +
> +		pr_debug("Getting an entry info rc = %d\n", rc);
> +	} else if (ac_entry_sub->ac_type == CXI_AC_GID) {
> +		rc = cxi_rgroup_get_ac_entry_id_by_data(ac_entry_sub->rgroup,
> +							CXI_AC_GID,
> +							&ac_entry_sub->ac_data,
> +							&ac_entry_id);
> +
> +		pr_debug("Getting an entry info rc = %d\n", rc);
> +	} else if (ac_entry_sub->ac_type == CXI_AC_NETNS) {
> +		rc = cxi_rgroup_get_ac_entry_id_by_data(ac_entry_sub->rgroup,
> +							CXI_AC_NETNS,
> +							&ac_entry_sub->ac_data,
> +							&ac_entry_id);
> +		pr_debug("Getting an entry info rc = %d\n", rc);
> +	} else {
> +		rc = cxi_rgroup_get_ac_entry_id_by_data(ac_entry_sub->rgroup,
> +							CXI_AC_OPEN,
> +							NULL,
> +							&ac_entry_id);
> +		pr_debug("Getting an entry info rc = %d\n", rc);
> +	}
> +
> +	if (!rc)
> +		cxi_rgroup_delete_ac_entry(ac_entry_sub->rgroup,
> +					   ac_entry_id);
> +	cxi_cfg_drop_item_ref(item);
> +}

[Severity: Medium]
Can the AC entry state in configfs get out of sync with the underlying
rgroup, tx_profile, and rx_profile entries, causing leaked entries or
deleting another configfs item's AC entry?

When a subdirectory is created in cxi_cfg_rgroup_ac_entry_make_group()
(and similarly in cxi_cfg_tx_prof_ac_entry_make_group() and
cxi_cfg_rx_prof_ac_entry_make_group()), ac_type is initialized to
CXI_AC_UID and ac_data.uid is 0 without adding a backend AC entry.

If that directory is removed via rmdir before type is written (or after
type_store fails with -EEXIST), cxi_cfg_rgroup_ac_entry_drop_item() and
cxi_cfg_tx_prof_ac_entry_drop_item() do not check whether this item added
an entry or delete by ac_entry_sub->id. Instead, they look up an entry by
(ac_type, ac_data) and delete it, which will delete another active item's
UID 0 entry.

In addition, configuring a CXI_AC_GID or CXI_AC_NETNS entry requires
writing type first because cxi_cfg_*_ac_entry_sub_data_store() checks
ac_entry_sub->ac_type. However, writing type immediately adds the backend
AC entry using the old ac_data (0), and writing data afterwards only
updates ac_entry_sub->ac_data without updating the backend entry. On
subsequent rmdir, looking up the entry by the new ac_data fails and leaks
the backend entry. Writing type more than once also overwrites
ac_entry_sub->id without deleting the previous entry, leaking it as well.

> +static ssize_t cxi_cfg_rgroup_res_type_max_show(struct config_item *item,
> +						char *page)
> +{
> +	struct cxi_cfg_rgroup_res_type *rgroup_res_type;
> +	struct cxi_resource_entry *entry;
> +	int rc;
> +
> +	rgroup_res_type = to_cxi_cfg_rgroup_res_type(item);
> +	rc = cxi_rgroup_get_resource_entry(rgroup_res_type->rgroup,
> +					   cxi_resource_str_to_type(rgroup_res_type->name),
> +					   &entry);
> +	if (rc != 0) {
> +		pr_debug("Error: Resource entry not yet created for the resource type\n");
> +		pr_debug("Error: Set the max file first and then the reserved file to create the resource type\n");
> +		return -EINVAL;
> +	}
> +
> +	return sprintf(page, "%ld\n", entry->limits.max);
> +}
> +
> +static ssize_t cxi_cfg_rgroup_res_type_max_store(struct config_item *item,
> +						 const char *page,
> +						 size_t count)
> +{
> +	struct cxi_cfg_rgroup_res_type *rgroup_res_type;
> +	struct cxi_resource_entry *entry;
> +	int rc;
> +
> +	rgroup_res_type = to_cxi_cfg_rgroup_res_type(item);
> +
> +	rc = cxi_rgroup_get_resource_entry(rgroup_res_type->rgroup,
> +					   cxi_resource_str_to_type(rgroup_res_type->name),
> +					   &entry);
> +	if (rc == 0) {
> +		pr_debug("Error: Given resource type has already been added\n");
> +		return -EINVAL;
> +	}
> +
> +	rc = kstrtoint(page, 0, &rgroup_res_type->max);
> +	if (rc)
> +		return rc;
> +
> +	pr_debug("resource max value set to: %d\n", rgroup_res_type->max);
> +
> +	return count;
> +}
> +
> +static ssize_t cxi_cfg_rgroup_res_type_reserved_show(struct config_item *item,
> +						     char *page)
> +{
> +	int rc;
> +	struct cxi_cfg_rgroup_res_type *rgroup_res_type;
> +	struct cxi_resource_entry *entry;
> +
> +	rgroup_res_type = to_cxi_cfg_rgroup_res_type(item);
> +
> +	rc = cxi_rgroup_get_resource_entry(rgroup_res_type->rgroup,
> +					   cxi_resource_str_to_type(rgroup_res_type->name),
> +					   &entry);
> +	if (rc != 0) {
> +		pr_debug("Error: Resource entry not found for the resource type\n");
> +		return -EINVAL;
> +	}
> +
> +	return sprintf(page, "%ld\n", entry->limits.reserved);
> +}
> +
> +static ssize_t cxi_cfg_rgroup_res_type_reserved_store(struct config_item *item,
> +						      const char *page,
> +						      size_t count)
> +{
> +	struct cxi_cfg_rgroup_res_type *rgroup_res_type;
> +	int rc;
> +	enum cxi_resource_type e_type;
> +	struct cxi_resource_limits limits = {};
> +
> +	rgroup_res_type = to_cxi_cfg_rgroup_res_type(item);
> +
> +	rc = kstrtoint(page, 0, &rgroup_res_type->reserved);
> +	if (rc)
> +		return rc;
> +
> +	e_type = cxi_resource_str_to_type(rgroup_res_type->name);
> +
> +	limits.max = rgroup_res_type->max;
> +	limits.reserved = rgroup_res_type->reserved;
> +
> +	rc = cxi_rgroup_add_resource(rgroup_res_type->rgroup, e_type, &limits);
> +	if (rc)
> +		return rc;
> +
> +	return count;
> +}
> +
> +static ssize_t cxi_cfg_rgroup_res_type_in_use_show(struct config_item *item,
> +						   char *page)
> +{
> +	struct cxi_cfg_rgroup_res_type *rgroup_res_type;
> +	struct cxi_resource_entry *entry;
> +	int rc;
> +
> +	rgroup_res_type = to_cxi_cfg_rgroup_res_type(item);
> +
> +	rc = cxi_rgroup_get_resource_entry(rgroup_res_type->rgroup,
> +					   cxi_resource_str_to_type(rgroup_res_type->name),
> +					   &entry);
> +	if (rc != 0) {
> +		pr_debug("Error: Given resource type has not yet been added\n");
> +		return -EINVAL;
> +	}
> +
> +	return sprintf(page, "%ld\n", entry->limits.in_use);
> +}

[Severity: Medium]
What happens if a negative value is written to
cxi_cfg_rgroup_res_type_max_store() or
cxi_cfg_rgroup_res_type_reserved_store()?

Because rgroup_res_type->max and rgroup_res_type->reserved are signed ints
parsed with kstrtoint(), negative values are accepted and then
sign-extended to size_t values near SIZE_MAX when assigned to limits.max
and limits.reserved. Also, entry->limits.max, entry->limits.reserved, and
entry->limits.in_use have type size_t in struct cxi_resource_limits, so
formatting them with "%ld\n" instead of "%zu\n" in the show callbacks
causes format warnings on 32-bit builds.

[ ... ]
> +static struct
> +config_group *cxi_cfg_rgroup_make_group(struct config_group *group,
> +					const char *name)
> +{
> +	struct cxi_cfg_rgroup *rgp = container_of(group,
> +						  struct cxi_cfg_rgroup,
> +						  group);
> +	struct cxi_cfg_rgroup_state *rgp_state;
> +	struct cxi_cfg_rgroup_attr *rgp_attr;
> +	struct cxi_cfg_rgroup_ac_entry *ac_entry;
> +	struct cxi_cfg_rgroup_res *rgp_res;
> +
> +	if (strncmp(name, "resources", sizeof("resources") - 1) == 0) {
> +		rgp_res = alloc_dev_rgroup_res("resources");
> +		if (!rgp_res) {
> +			pr_debug("unable to alloc rgroup resources");
> +			return ERR_PTR(-EINVAL);
> +		}
> +		rgp_res->rgroup = rgp->rgroup;
> +		rgp->rgp_res = rgp_res;
> +		rgp->rgp_res->cdev = rgp->cdev;
> +
> +		return &rgp->rgp_res->group;
> +	}
> +
> +	if (strncmp(name, "attr", sizeof("attr") - 1) == 0) {
> +		rgp_attr = alloc_dev_rgroup_attr("attr");
> +		if (!rgp_attr) {
> +			pr_debug("unable to alloc rgroup attr");
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		rgp_attr->rgroup = rgp->rgroup;
> +		rgp->rgp_attr = rgp_attr;
> +		rgp->rgp_attr->cdev = rgp->cdev;
> +
> +		return &rgp->rgp_attr->group;
> +	}
> +
> +	if (strncmp(name, "state", sizeof("state") - 1) == 0) {
> +		rgp_state = alloc_dev_rgroup_state("state");
> +		if (!rgp_state) {
> +			pr_debug("unable to alloc rgroup state");
> +			return ERR_PTR(-EINVAL);
> +		}
> +		rgp_state->rgroup = rgp->rgroup;
> +		rgp->rgp_state = rgp_state;
> +		rgp->rgp_state->cdev = rgp->cdev;
> +
> +		return &rgp->rgp_state->group;
> +	}
> +
> +	if (strncmp(name, "ac-entry", sizeof("ac-entry") - 1) == 0) {
> +		ac_entry = alloc_dev_rgroup_ac_entry("ac-entry");
> +		if (!ac_entry) {
> +			pr_debug("unable to alloc rgroup ac-entry");
> +			return ERR_PTR(-EINVAL);
> +		}
> +		ac_entry->rgroup = rgp->rgroup;
> +		rgp->ac_entry = ac_entry;
> +
> +		return &rgp->ac_entry->group;
> +	}
> +
> +	return ERR_PTR(-EINVAL);
> +}

[Severity: Medium]
Should cxi_cfg_rgroup_make_group(), cxi_cfg_tx_prof_make_group(), and
cxi_cfg_rx_prof_make_group() use strcmp() instead of
strncmp(name, ..., sizeof(...) - 1)?

Using sizeof(...) - 1 only compares the prefix without the NUL terminator,
allowing userspace to mkdir names with arbitrary suffixes such as
attr_extra while the allocated config_group has ci_name hardcoded to
"attr". If userspace then creates a second directory with the same prefix,
make_group() overwrites the parent pointer (such as rgp->rgp_attr) before
configfs_attach_group() -> configfs_dirent_exists() fails with -EEXIST and
frees the newly allocated struct, leaving the parent pointer dangling.

Also, on allocation failure in cxi_cfg_rgroup_make_group(), should these
branches return ERR_PTR(-ENOMEM) instead of ERR_PTR(-EINVAL)?

[ ... ]
> +static struct
> +config_group *cxi_cfg_rgroup_dir_make_group(struct config_group *group,
> +					    const char *name)
> +{
[ ... ]
> +	rgp->cdev = dev_rgp->cdev;
> +	strscpy(attr.name, name, sizeof(attr.name));
> +	rgroup = cxi_dev_alloc_rgroup(dev_rgp->cdev,
> +				      &attr);
> +	if (IS_ERR(rgroup)) {
> +		kfree(rgp);
> +		return ERR_PTR(PTR_ERR(rgroup));
> +	}
> +	rgp->rgroup = rgroup;
> +
> +	return &rgp->group;
> +}

[Severity: Medium]
Does calling kfree(rgp) here (and similarly kfree(tx_prof) in
cxi_cfg_tx_prof_dir_make_group(), kfree(rx_prof) in
cxi_cfg_rx_prof_dir_make_group(), and kfree() in the error unwind paths of
cxi_configfs_device_init()) leak group.cg_item.ci_name when strlen(name) is
20 or more characters?

alloc_dev_rgroup(), alloc_dev_tx_prof(), alloc_dev_rx_prof(), and
alloc_dev_subdir() call config_group_init_type_name(), which dynamically
allocates ci_name when strlen(name) >= CONFIGFS_ITEM_NAME_LEN (20).
Calling kfree() directly instead of config_item_put(&...->group.cg_item)
bypasses config_item_cleanup() and leaks ci_name.

[ ... ]
> +static ssize_t cxi_cfg_tx_prof_ac_entry_sub_type_store(struct config_item *item,
> +						       const char *page,
> +						       size_t count)
> +{
> +	struct cxi_cfg_tx_prof_ac_entry_sub *ac_entry_sub;
> +	int rc;
> +	enum cxi_ac_type ac_type_val;
> +	uid_t uid;
> +	gid_t gid;
> +	unsigned int netns;
> +	unsigned int ac_entry_id;
> +
> +	ac_entry_sub = to_cxi_cfg_tx_prof_ac_entry_sub(item);
> +	if (!ac_entry_sub->tx_profile) {
> +		pr_debug("Unable to get tx profile reference\n");
> +		return -EINVAL;
> +	}
> +
> +	rc = kstrtouint(page, 0, (unsigned int *)&ac_type_val);
> +	if (rc)
> +		return rc;
> +
> +	if (ac_type_val == CXI_AC_UID)
> +		uid = ac_entry_sub->ac_data.uid;
> +	else if (ac_type_val == CXI_AC_GID)
> +		gid = ac_entry_sub->ac_data.gid;
> +	else if (ac_type_val == CXI_AC_NETNS)
> +		netns = ac_entry_sub->ac_data.netns;
> +	else if (ac_type_val == CXI_AC_OPEN)
> +		pr_debug("Setting type to CXI_AC_OPEN\n");
> +	else
> +		return -EINVAL;
> +
> +	rc = cxi_tx_profile_add_ac_entry(ac_entry_sub->tx_profile,
> +					 ac_type_val,
> +					 uid,
> +					 gid,
> +					 netns,
> +					 &ac_entry_id);

[Severity: Medium]
Are uid, gid, and netns passed uninitialized to
cxi_tx_profile_add_ac_entry() here (and to cxi_rx_profile_add_ac_entry() in
cxi_cfg_rx_prof_ac_entry_sub_type_store())?

Only one of uid, gid, or netns (or none when ac_type_val == CXI_AC_OPEN) is
initialized before all three variables are passed by value.

[ ... ]
> +static ssize_t cxi_cfg_tx_prof_attr_match_store(struct config_item *item,
> +						const char *page,
> +						size_t count)
> +{
> +	struct cxi_cfg_tx_prof_attr *tx_prof_attr;
> +	int rc;
> +	struct cxi_tx_attr tx_attr = {};
> +
> +	tx_prof_attr = to_cxi_cfg_tx_prof_attr(item);
> +
> +	rc = kstrtou16(page, 0, &tx_prof_attr->match);
> +	if (rc)
> +		return rc;
> +
> +	tx_attr.vni_attr.match = tx_prof_attr->match;
> +	tx_attr.vni_attr.ignore = tx_prof_attr->ignore;
> +
> +	strscpy(tx_attr.vni_attr.name,
> +		tx_prof_attr->tx_profile->profile_common.vni_attr.name,
> +		sizeof(tx_attr.vni_attr.name));
> +
> +	rc = cxi_dev_set_tx_profile_attr(tx_prof_attr->cdev,
> +					 tx_prof_attr->tx_profile,
> +					 &tx_attr);
> +	if (rc)
> +		return rc;
> +
> +	return count;
> +}

[Severity: Medium]
If cxi_dev_set_tx_profile_attr() fails in
cxi_cfg_tx_prof_attr_match_store() (or cxi_dev_set_rx_profile_attr() fails
in cxi_cfg_rx_prof_attr_match_store()), does tx_prof_attr->match retain the
rejected value?

Unlike cxi_cfg_tx_prof_attr_ignore_store() and
cxi_cfg_rx_prof_attr_ignore_store(), which parse into a local variable and
query current profile attributes via cxi_tx_profile_get_info() /
cxi_rx_profile_get_info() before applying the update, the match_store
functions write directly to tx_prof_attr->match / rx_prof_attr->match and
use the cached ignore field (which is 0 if the attr directory was removed
and recreated).

[ ... ]
> +static void set_tc_bool_type(unsigned int tc_type,
> +			     struct cxi_cfg_tx_prof_tc *tx_prof_tc,
> +			     bool tc_bool)
> +{
> +	switch (tc_type) {
> +	case CXI_TC_BEST_EFFORT:
> +		tx_prof_tc->best_effort = tc_bool;
> +		break;
> +	case CXI_TC_DEDICATED_ACCESS:
> +		tx_prof_tc->dedicated = tc_bool;
> +		break;
> +	case CXI_TC_LOW_LATENCY:
> +		tx_prof_tc->low_latency = tc_bool;
> +		break;
> +	case CXI_TC_BULK_DATA:
> +		tx_prof_tc->bulk_data = tc_bool;
> +		break;
> +	case CXI_TC_ETH:
> +		tx_prof_tc->eth = tc_bool;
> +		break;
> +	}
> +
> +	cxi_tx_profile_set_tc(tx_prof_tc->tx_profile, tc_type, tc_bool);
> +}
[ ... ]
> +static ssize_t cxi_cfg_tx_prof_tc_best_effort_show(struct config_item *item,
> +						   char *page)
> +{
> +	struct cxi_cfg_tx_prof_tc *tx_prof_tc;
> +
> +	tx_prof_tc = to_cxi_cfg_tx_prof_tc(item);
> +
> +	return scnprintf(page, PAGE_SIZE, "%s\n",
> +			 tx_prof_tc->best_effort ? "true" : "false");
> +}

[Severity: Low]
Can the traffic-class show callbacks in cxi_cfg_tx_prof_tc report stale
values if the tc configfs directory is removed and recreated?

Because cxi_cfg_tx_prof_drop_item() does not clear
tx_profile->config.tc_table and alloc_dev_tx_prof_tc() zero-initializes
struct cxi_cfg_tx_prof_tc, reading tx_prof_tc->best_effort (and dedicated,
low_latency, bulk_data, eth) instead of querying
cxi_tx_profile_valid_tc(tx_prof_tc->tx_profile, tc) will report false for
traffic classes that are still enabled on the profile.

[ ... ]
> +int cxi_configfs_device_init(struct cass_dev *hw)
> +{
[ ... ]
> +	rc = configfs_register_group(&cxi_cfg_configfs->group,
> +				     &rgp_dir->group);
> +	if (rc)
> +		goto rgp_dir_reg_err;
> +
> +	cxi_cfg_configfs->rgp_dir = rgp_dir;
> +	rgp_dir->cdev = &hw->cdev;
> +
> +	rx_prof_dir = alloc_dev_rx_prof_dir("rx-profile");
> +	if (!rx_prof_dir) {
> +		rc = -ENOMEM;
> +		goto rx_prof_dir_alloc_err;
> +	}
> +
> +	rc = configfs_register_group(&cxi_cfg_configfs->group,
> +				     &rx_prof_dir->group);
> +	if (rc)
> +		goto rx_prof_dir_reg_err;
> +
> +	cxi_cfg_configfs->rx_prof_dir = rx_prof_dir;
> +	rx_prof_dir->cdev = &hw->cdev;
> +
> +	tx_prof_dir = alloc_dev_tx_prof_dir("tx-profile");
> +	if (!tx_prof_dir) {
> +		rc = -ENOMEM;
> +		goto tx_prof_dir_alloc_err;
> +	}
> +
> +	rc = configfs_register_group(&cxi_cfg_configfs->group,
> +				     &tx_prof_dir->group);
> +	if (rc)
> +		goto tx_prof_dir_reg_err;
> +
> +	cxi_cfg_configfs->tx_prof_dir = tx_prof_dir;
> +	tx_prof_dir->cdev = &hw->cdev;

[Severity: Medium]
In cxi_configfs_device_init(), can a concurrent mkdir race with group
registration because rgp_dir->cdev, rx_prof_dir->cdev, and
tx_prof_dir->cdev are assigned after configfs_register_group() returns?

Once configfs_register_group() marks the directory ready, userspace can
immediately invoke cxi_cfg_rgroup_dir_make_group(),
cxi_cfg_rx_prof_dir_make_group(), or cxi_cfg_tx_prof_dir_make_group()
while ->cdev is still NULL, passing NULL into cxi_dev_alloc_rgroup(),
cxi_dev_alloc_rx_profile(), or cxi_dev_alloc_tx_profile().

[ ... ]
> +static struct config_group *
> +get_first_child_config_group(struct config_group *parent_group,
> +			     struct cxi_cfg_teardown_marker *marker)
> +{
> +	struct config_item *child_item;
> +	struct config_group *child_group = NULL;
> +	unsigned long flags;
> +
> +	mutex_lock(&cxi_cfg_group_subsys.su_mutex);
> +	child_item = list_first_entry_or_null(&parent_group->cg_children,
> +					      struct config_item, ci_entry);
> +	if (child_item) {
> +		config_item_get(child_item);
> +		marker->item = child_item;
> +		INIT_LIST_HEAD(&marker->list);
> +		spin_lock_irqsave(&cxi_cfg_teardown_lock, flags);
> +		list_add(&marker->list, &cxi_cfg_teardown_markers);
> +		spin_unlock_irqrestore(&cxi_cfg_teardown_lock, flags);
> +		child_group = container_of(child_item,
> +					   struct config_group, cg_item);
> +	}
> +	mutex_unlock(&cxi_cfg_group_subsys.su_mutex);
> +
> +	return child_group;
> +}
> +
> +static void unregister_all_sub_config_groups_recur(struct config_group *parent_group)
> +{
> +	struct config_group *child_group;
> +	struct cxi_cfg_teardown_marker marker;
> +	unsigned long flags;
> +
> +	/*
> +	 * Unregistering a child unlinks it from cg_children, so the list
> +	 * is re-read each time round instead of being walked across the
> +	 * unlocked section.
> +	 */
> +	while ((child_group = get_first_child_config_group(parent_group, &marker))) {
> +		unregister_all_sub_config_groups_recur(child_group);
> +		configfs_unregister_group(child_group);
> +
> +		spin_lock_irqsave(&cxi_cfg_teardown_lock, flags);
> +		list_del_init(&marker.list);
> +		spin_unlock_irqrestore(&cxi_cfg_teardown_lock, flags);
> +
> +		/* The reference taken by get_first_child_config_group() */
> +		config_item_put(&child_group->cg_item);
> +		/* The creator's reference, as before */
> +		config_item_put(&child_group->cg_item);
> +	}
> +}

[Severity: High]
Is it safe for unregister_all_sub_config_groups_recur() to call
configfs_unregister_group() on user-created mkdir groups after dropping
cxi_cfg_group_subsys.su_mutex?

First, if userspace concurrently runs rmdir on child_group after
get_first_child_config_group() drops su_mutex, configfs_rmdir() detaches
the dentry, calls unlink_group() (setting child_group->cg_subsys = NULL and
child_group->cg_item.ci_parent = NULL), and calls client_drop_item() ->
cxi_cfg_drop_item_ref(), which sees the teardown marker and skips
config_item_put(). When unregister_all_sub_config_groups_recur() then calls
configfs_unregister_group(child_group), it dereferences
group->cg_item.ci_parent->ci_dentry and subsys->su_mutex while both
ci_parent and subsys are NULL.

Second, configfs_unregister_group() is meant for default groups registered
with configfs_register_group(). Calling it on mkdir-created groups does not
invoke .drop_item callbacks (skipping cxi_rgroup_dec_refcount(),
cxi_tx_profile_dec_refcount(), cxi_rx_profile_dec_refcount(),
cxi_rgroup_delete_resource(), and AC entry removal) and never releases the
two module references taken by configfs_mkdir() on subsys_owner and
new_item_owner, preventing module unload.

> diff --git a/drivers/net/ethernet/hpe/ss1/cxi_core.c b/drivers/net/ethernet/hpe/ss1/cxi_core.c
> new file mode 100644
> index 0000000000000..d46ab3a1df104
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/ss1/cxi_core.c
> @@ -0,0 +1,281 @@
[ ... ]
> +unsigned int pid_bits = DEFAULT_PID_BITS;
> +module_param(pid_bits, uint, 0444);
> +MODULE_PARM_DESC(pid_bits, "Width of the PID field within a NIC address");
> +
> +/* System info to determine if a system is mixed (C1 & C2)
> + * or homogeneous (only C1 or only C2).
> + */
> +unsigned int system_type_identifier;
> +module_param(system_type_identifier, uint, 0444);
> +MODULE_PARM_DESC(system_type_identifier,
> +		 "Identifies what type of Cassini NICs are present in the system\n"
> +		"0 - Cassini 1 and Cassini 2 (Default)\n"
> +		"1 - Cassini 1 Only\n"
> +		"2 - Cassini 2 Only\n");
> +
> +unsigned int min_free_shift;
> +module_param(min_free_shift, uint, 0444);
> +MODULE_PARM_DESC(min_free_shift, "min_free shift (0 to 8)");
> +
> +unsigned int vni_matching;
> +module_param(vni_matching, uint, 0444);
> +MODULE_PARM_DESC(vni_matching, "vni matching configuration (0 to 3)");
> +
> +unsigned int tg_threshold[4] = { 0, 64, 128, 256 };
> +module_param_array(tg_threshold, uint, NULL, 0644);
> +MODULE_PARM_DESC(tg_threshold, "Array of 4 LPE append credits. Credits are 0 for immediate append, or 4 to 16383 for delayed append.");

[Severity: Medium]
Since tg_threshold is registered with 0644 permissions without custom
param_ops, what happens if userspace modifies tg_threshold via sysfs after
cxi_init() has run?

Runtime writes bypass the 0 or 4..16383 check in cxi_init(), can exceed
INT_MAX and wrap to a negative value when returned by
cxi_get_lpe_append_credits(), and do not update the hardware
C_CQ_CFG_TG_THRESH register programmed during cass_cq_init(), leaving
software LE credit reservations out of sync with hardware.

> +
> +int untagged_eth_pcp = -1;
> +module_param(untagged_eth_pcp, int, 0444);
> +MODULE_PARM_DESC(untagged_eth_pcp, "PCP used by untagged Ethernet frames. Valid PCPs 0-7. Use -1 to use the PCP specified in chosen QOS Profile");
> +
> +bool switch_connected = true;
> +module_param(switch_connected, bool, 0644);
> +MODULE_PARM_DESC(switch_connected, "Connected to a switch or another NIC");

[Severity: Low]
Should these seven module parameters (pid_bits, system_type_identifier,
min_free_shift, vni_matching, tg_threshold, untagged_eth_pcp, and
switch_connected) be documented in
Documentation/admin-guide/kernel-parameters.txt?

Also, in cxi_init(), untagged_eth_pcp is a signed int checked against < -1,
so should the error message format it with %d instead of %u?

[ ... ]
> +/* Send an asynchronous event to the clients */
> +void cxi_send_async_event(struct cxi_dev *cdev, enum cxi_async_event event)
> +{
> +	struct cxi_client *client;
> +
> +	mutex_lock_nested(&dev_list_mutex, SINGLE_DEPTH_NESTING);
> +
> +	list_for_each_entry(client, &client_list, list)
> +		if (client->async_event)
> +			client->async_event(cdev, event);
> +
> +	mutex_unlock(&dev_list_mutex);
> +}

[Severity: High]
Why does cxi_send_async_event() call mutex_lock_nested(&dev_list_mutex,
SINGLE_DEPTH_NESTING) on the single static dev_list_mutex instance?

A struct mutex cannot be locked recursively by the same task; if
cxi_send_async_event() is called while dev_list_mutex is already held, it
will self-deadlock at runtime. Using SINGLE_DEPTH_NESTING on the same
singleton mutex also masks lock ordering inversions involving
dev_list_mutex from lockdep, such as dev_list_mutex -> rtnl_lock in
cxi_add_device() / cxi_remove_device() versus rtnl_lock -> dev_list_mutex
when ndo_set_mac_address calls cxi_eth_mac_addr() ->
cxi_set_nid_from_mac() -> program_nid() -> cxi_send_async_event().

[ ... ]
> +static int __init cxi_init(void)
> +{
[ ... ]
> +	rc = hw_register();
> +	if (rc < 0) {
> +		pr_err("Driver registration failed\n");
> +		goto err_drv_reg;
> +	}
> +
> +	if (!(system_type_identifier == CASSINI_1_ONLY ||
> +	      system_type_identifier == CASSINI_2_ONLY ||
> +		system_type_identifier == CASSINI_MIX)) {
> +		pr_warn("Unknown system_type_identifier (%u). Using default (%u).\n",
> +			system_type_identifier, CASSINI_MIX);
> +		system_type_identifier = CASSINI_MIX;
> +	}

[Severity: Medium]
Should system_type_identifier be validated before calling hw_register() in
cxi_init()?

hw_register() calls pci_register_driver(), which synchronously runs
cass_probe() -> init_hw() -> cass_oxe_init() for devices already present.
If an invalid system_type_identifier is passed, cass_probe() and
cass_oxe_init() consume the unvalidated value (failing to disable
C_OXE_CFG_COMMON.ioi_enable) before cxi_init() resets
system_type_identifier to CASSINI_MIX.

> diff --git a/drivers/net/ethernet/hpe/ss1/cxi_link.c b/drivers/net/ethernet/hpe/ss1/cxi_link.c
> new file mode 100644
> index 0000000000000..8d53bbb2f027d
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/ss1/cxi_link.c
[ ... ]
> +void cxi_pml_recovery_set(struct cxi_dev *cxi_dev, bool set)
> +{
> +	struct cass_dev *hw = container_of(cxi_dev, struct cass_dev, cdev);
> +
> +	hw->link_ops->pml_recovery_set(hw, set);
> +}
> +EXPORT_SYMBOL(cxi_pml_recovery_set);

[Severity: Medium]
Does cxi_pml_recovery_set() have inverted boolean polarity between Cassini 1
(cxi_link_ops_sbl) and Cassini 2 (cxi_link_ops_sl)?

On Cassini 2, cass_sl_pml_recovery_set(hw, true) sets
SL_LINK_CONFIG_OPT_PML_REC_ENABLE (enabling PML recovery), whereas on
Cassini 1, cass_sbl_pml_recovery_set(hw, true) calls
sbl_disable_pml_recovery(hw->sbl, 0, true), which sets
SBL_DISABLE_PML_RECOVERY (disabling PML recovery).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-cxi-netdev-submission-v1-0-8c2c391cc283@hpe.com?part=20