Re: [PATCH net] dpll: fix device-id-get and pin-id-get to return errors properly

[Ivan Vecera <ivecera@redhat.com>]

On 10/24/25 9:07 PM, Petr Oros wrote:
> The device-id-get and pin-id-get handlers were ignoring errors from
> the find functions and sending empty replies instead of returning
> error codes to userspace.
> 
> When dpll_device_find_from_nlattr() or dpll_pin_find_from_nlattr()
> returned an error (e.g., -EINVAL for "multiple matches" or -ENODEV
> for "not found"), the handlers checked `if (!IS_ERR(ptr))` and
> skipped adding the device/pin handle to the message, but then still
> sent the empty message as a successful reply.
> 
> This caused userspace tools to receive empty responses with id=0
> instead of proper netlink errors with extack messages like
> "multiple matches".
> 
> The bug is visible via strace, which shows the kernel sending TWO
> netlink messages in response to a single request:
> 
> 1. Empty reply (20 bytes, just header, no attributes):
>     recvfrom(3, [{nlmsg_len=20, nlmsg_type=dpll, nlmsg_flags=0, ...},
>                  {cmd=0x7, version=1}], ...)
> 
> 2. NLMSG_ERROR ACK with extack (because of NLM_F_ACK flag):
>     recvfrom(3, [{nlmsg_len=60, nlmsg_type=NLMSG_ERROR,
>                   nlmsg_flags=NLM_F_CAPPED|NLM_F_ACK_TLVS, ...},
>                  [{error=0, msg={...}},
>                   [{nla_type=NLMSGERR_ATTR_MSG}, "multiple matches"]]], ...)
> 
> The C YNL library parses the first message, sees an empty response,
> and creates a result object with calloc() which zero-initializes all
> fields, resulting in id=0.
> 
> The Python YNL library parses both messages and displays the extack
> from the second NLMSG_ERROR message.
> 
> Fix by checking `if (IS_ERR(ptr))` first and returning the error
> code immediately, so that netlink properly sends only NLMSG_ERROR with
> the extack message to userspace. After this fix, both C and Python
> YNL tools receive only the NLMSG_ERROR and behave consistently.
> 
> This affects:
> - DPLL_CMD_DEVICE_ID_GET: now properly returns error when multiple
>    devices match the criteria (e.g., same module-name + clock-id)
> - DPLL_CMD_PIN_ID_GET: now properly returns error when multiple pins
>    match the criteria (e.g., same module-name)
> 
> Before fix:
>    $ dpll pin id-get module-name ice
>    0  (wrong - should be error, there are 17 pins with module-name "ice")
> 
> After fix:
>    $ dpll pin id-get module-name ice
>    Error: multiple matches
>    (correct - kernel reports the ambiguity via extack)
> 
> Signed-off-by: Petr Oros <poros@redhat.com>

Reviewed-by: Ivan Vecera <ivecera@redhat.com>

> ---
>   drivers/dpll/dpll_netlink.c | 36 ++++++++++++++++++++----------------
>   1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
> index 74c1f0ca95f24a..a4153bcb6dcfe1 100644
> --- a/drivers/dpll/dpll_netlink.c
> +++ b/drivers/dpll/dpll_netlink.c
> @@ -1559,16 +1559,18 @@ int dpll_nl_pin_id_get_doit(struct sk_buff *skb, struct genl_info *info)
>   		return -EMSGSIZE;
>   	}
>   	pin = dpll_pin_find_from_nlattr(info);
> -	if (!IS_ERR(pin)) {
> -		if (!dpll_pin_available(pin)) {
> -			nlmsg_free(msg);
> -			return -ENODEV;
> -		}
> -		ret = dpll_msg_add_pin_handle(msg, pin);
> -		if (ret) {
> -			nlmsg_free(msg);
> -			return ret;
> -		}
> +	if (IS_ERR(pin)) {
> +		nlmsg_free(msg);
> +		return PTR_ERR(pin);
> +	}
> +	if (!dpll_pin_available(pin)) {
> +		nlmsg_free(msg);
> +		return -ENODEV;
> +	}
> +	ret = dpll_msg_add_pin_handle(msg, pin);
> +	if (ret) {
> +		nlmsg_free(msg);
> +		return ret;
>   	}
>   	genlmsg_end(msg, hdr);
>   
> @@ -1735,12 +1737,14 @@ int dpll_nl_device_id_get_doit(struct sk_buff *skb, struct genl_info *info)
>   	}
>   
>   	dpll = dpll_device_find_from_nlattr(info);
> -	if (!IS_ERR(dpll)) {
> -		ret = dpll_msg_add_dev_handle(msg, dpll);
> -		if (ret) {
> -			nlmsg_free(msg);
> -			return ret;
> -		}
> +	if (IS_ERR(dpll)) {
> +		nlmsg_free(msg);
> +		return PTR_ERR(dpll);
> +	}
> +	ret = dpll_msg_add_dev_handle(msg, dpll);
> +	if (ret) {
> +		nlmsg_free(msg);
> +		return ret;
>   	}
>   	genlmsg_end(msg, hdr);
>