Re: [RFC PATCH 09/12] drm/fabric: implement mutation netlink operations

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Mutation operations (NEW/ADD/SET) only return an ACK instead of replying with a full message.
- [Medium] Notifications emitted in response to mutation requests do not honor the NLM_F_ECHO flag.
- [Medium] Extraneous wrapping nest used for FABRIC_NEW parameters.
- [Low] Missing extended ACK coverage when required nested attributes in peer descriptors are absent.
--

--- Patch [9]: [RFC PATCH 09/12] drm/fabric: implement mutation netlink operations ---
commit 7000874bd5559d10291fdcd411ac9f167a4334c9
Author: Konstantin Sinyuk <[email protected]>

    drm/fabric: implement mutation netlink operations
    
    Connect the six provisioning operations to the core mutation helpers and
    add the generated operation and policy source that dispatches to them.
    
    Resolve and pin mutation targets in pre_doit, run each transaction under
    drm_fabric_mutation_lock, call the provider without drm_fabric_lock held,
    and release references in post_doit. All six require GENL_ADMIN_PERM and
    are confined to init_net.
    
    A failed provider callback returns its error with core state unchanged.
    Successful mutations emit notifications after commit, carrying the
    resulting topology generation.

> diff --git a/drivers/gpu/drm/fabric/drm_fabric_netlink.c b/drivers/gpu/drm/fabric/drm_fabric_netlink.c
> index fef2d4c8f5bb9..1a0f293723e95 100644
> --- a/drivers/gpu/drm/fabric/drm_fabric_netlink.c
> +++ b/drivers/gpu/drm/fabric/drm_fabric_netlink.c
[ ... ]
> +/* A nested policy cannot mark members required; check type and instance-id here. */
> +static int drm_fabric_parse_new_params(struct genl_info *info,
> +					enum drm_fabric_type *type,
> +					const char **name, u64 *instance_id)
> +{
> +	struct nlattr *pa[DRM_FABRIC_A_FABRIC_NEW_PARAMS_MAX + 1];
> +	struct nlattr *nest;
> +	int ret;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, DRM_FABRIC_A_FABRIC_NEW_PARAMS))
> +		return -EINVAL;
> +
> +	nest = info->attrs[DRM_FABRIC_A_FABRIC_NEW_PARAMS];
> +	ret = nla_parse_nested(pa, DRM_FABRIC_A_FABRIC_NEW_PARAMS_MAX, nest,
> +			       drm_fabric_fabric_new_params_nl_policy,
> +			       info->extack);

[Severity: Medium]
Does this create an extra wrapping nest for the new parameters?

Netlink uAPI guidelines for new families prefer flat attributes over wrapping
nests. Is it possible to avoid using DRM_FABRIC_A_FABRIC_NEW_PARAMS and instead
place these parameters directly in the main attribute set?

[ ... ]
> +int drm_fabric_nl_endpoint_set_doit(struct sk_buff *skb,
> +				    struct genl_info *info)
> +{
[ ... ]
> +	if (!change.valid)
> +		return -EINVAL;
> +
> +	return drm_fabric_endpoint_set(ep, &change);
> +}

[Severity: Medium]
Should this function reply with a full message rather than just an ACK?

New netlink families should reply to mutation operations (like SET and NEW)
with a full message carrying the object state, rather than just relying on
a generic ACK. This applies here as well as to drm_fabric_nl_port_set_doit()
and drm_fabric_nl_port_peer_new_doit().

[ ... ]
> +int drm_fabric_nl_port_peer_new_doit(struct sk_buff *skb,
> +				     struct genl_info *info)
> +{
> +	struct drm_fabric_port *port = info->user_ptr[0];
> +	struct nlattr *pa[DRM_FABRIC_A_PEER_ATTRS_MAX + 1];
> +	struct drm_fabric_peer peer = {};
> +	int ret;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, DRM_FABRIC_A_PEER))
> +		return -EINVAL;
> +
> +	ret = nla_parse_nested(pa, DRM_FABRIC_A_PEER_ATTRS_MAX,
> +			       info->attrs[DRM_FABRIC_A_PEER],
> +			       drm_fabric_peer_nl_policy, info->extack);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * A nested policy cannot require members; require the complete peer
> +	 * descriptor here.
> +	 */
> +	if (!pa[DRM_FABRIC_A_PEER_ATTRS_PEER_ID] ||
> +	    !pa[DRM_FABRIC_A_PEER_ATTRS_TYPE] ||
> +	    !pa[DRM_FABRIC_A_PEER_ATTRS_PORT_INDEX])
> +		return -EINVAL;

[Severity: Low]
Could this manual check use NL_REQ_ATTR_CHECK() to provide extended ACK
information?

Currently, this returns a bare -EINVAL without populating the extack to inform
userspace which specific nested attribute was missing from the peer descriptor.

> +
> +	peer.peer_id = nla_get_u64(pa[DRM_FABRIC_A_PEER_ATTRS_PEER_ID]);
> +	peer.peer_type = nla_get_u32(pa[DRM_FABRIC_A_PEER_ATTRS_TYPE]);
> +	peer.port_index = nla_get_u32(pa[DRM_FABRIC_A_PEER_ATTRS_PORT_INDEX]);
> +
> +	return drm_fabric_port_peer_new(port, &peer);
> +}
[ ... ]
> +void drm_fabric_emit_endpoint_change(struct drm_fabric_endpoint *ep, u32 generation)
> +{
> +	drm_fabric_endpoint_event_send(DRM_FABRIC_CMD_ENDPOINT_CHANGE_NTF, ep,
> +				       generation);
> +}

[Severity: Medium]
Will these notifications honor the NLM_F_ECHO flag?

The event send functions (such as drm_fabric_endpoint_event_send() called here,
and drm_fabric_emit_port_change()) use genlmsg_multicast() directly without
the originating request context.

When emitting a notification in response to a request, passing the request info
to genl_notify() ensures that userspace applications setting NLM_F_ECHO will
correctly receive their own mutations back.

> diff --git a/drivers/gpu/drm/fabric/drm_fabric_nl.c b/drivers/gpu/drm/fabric/drm_fabric_nl.c
> index 0325484051466..20b277f27ae9b 100644
> --- a/drivers/gpu/drm/fabric/drm_fabric_nl.c
> +++ b/drivers/gpu/drm/fabric/drm_fabric_nl.c
[ ... ]
> +/* DRM_FABRIC_CMD_FABRIC_NEW - do */
> +static const struct nla_policy drm_fabric_fabric_new_nl_policy[DRM_FABRIC_A_FABRIC_NEW_PARAMS + 1] = {
> +	[DRM_FABRIC_A_FABRIC_NEW_PARAMS] = NLA_POLICY_NESTED(drm_fabric_fabric_new_params_nl_policy),
> +};

[Severity: Medium]
Does this policy enforce an extraneous wrapping nest?

As mentioned earlier, Netlink uAPI guidelines discourage wrapping nests for
new families. Can these parameters be flattened into the main attribute set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9