| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 24 Jul 2023 18:34:06 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: Jakub Kicinski <kuba@...nel.org>, Jiri Pirko <jiri@...nulli.us>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
Jonathan Lemon <jonathan.lemon@...il.com>,
Paolo Abeni <pabeni@...hat.com>,
Milena Olech <milena.olech@...el.com>,
Michal Michalik <michal.michalik@...el.com>,
linux-arm-kernel@...ts.infradead.org, poros@...hat.com,
mschmidt@...hat.com, netdev@...r.kernel.org,
linux-clk@...r.kernel.org, Bart Van Assche <bvanassche@....org>
Subject: Re: [PATCH net-next 06/11] dpll: netlink: Add DPLL framework base
functions
On Thu, Jul 20, 2023 at 10:18:58AM +0100, Vadim Fedorenko wrote:
> DPLL framework is used to represent and configure DPLL devices
> in systems. Each device that has DPLL and can configure inputs
> and outputs can use this framework.
Hi Vadim,
some minor feedback from my side.
>
> Implement dpll netlink framework functions for enablement of dpll
> subsytem netlink family.
subsytem -> subsystem
...
> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
...
> +/**
> + * dpll_msg_add_pin_handle - attach pin handle attribute to a given message
> + * @msg: pointer to sk_buff message to attach a pin handle
> + * @pin: pin pointer
> + *
> + * Return:
> + * * 0 - success
> + * * -EMSGSIZE - no space in message to attach pin handle
> + */
> +int dpll_msg_add_pin_handle(struct sk_buff *msg, struct dpll_pin *pin)
This function seems to only be used in this file.
Should it be static.
...
> +static int
> +dpll_msg_add_pin_on_dpll_state(struct sk_buff *msg, struct dpll_pin *pin,
> + struct dpll_pin_ref *ref,
> + struct netlink_ext_ack *extack)
> +{
> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> + struct dpll_device *dpll = ref->dpll;
> + enum dpll_pin_state state;
> + int ret;
> +
> +
nit: one blank line is enough
...
> +static int
> +dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
> + struct dpll_pin_ref *ref, struct netlink_ext_ack *extack)
> +{
> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> + struct dpll_device *dpll = ref->dpll;
> + struct nlattr *nest;
> + int fs, ret;
> + u64 freq;
> +
> + if (!ops->frequency_get)
> + return 0;
> + ret = ops->frequency_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
> + dpll_priv(dpll), &freq, extack);
> + if (ret)
> + return ret;
> + if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY, sizeof(freq), &freq, 0))
> + return -EMSGSIZE;
> + for (fs = 0; fs < pin->prop->freq_supported_num; fs++) {
> + nest = nla_nest_start(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED);
> + if (!nest)
> + return -EMSGSIZE;
> + freq = pin->prop->freq_supported[fs].min;
> + if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN, sizeof(freq),
> + &freq, 0)) {
nit: The indention of the line above isn't quite right.
There is one space too many.
> + nla_nest_cancel(msg, nest);
> + return -EMSGSIZE;
> + }
> + freq = pin->prop->freq_supported[fs].max;
> + if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX, sizeof(freq),
> + &freq, 0)) {
Ditto.
> + nla_nest_cancel(msg, nest);
> + return -EMSGSIZE;
> + }
> + nla_nest_end(msg, nest);
> + }
> +
> + return 0;
> +}
...
> +static int
> +dpll_device_event_send(enum dpll_cmd event, struct dpll_device *dpll)
> +{
> + struct sk_buff *msg;
> + void *hdr;
> + int ret;
> +
> + if (WARN_ON(!xa_get_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED)))
> + return -ENODEV;
> + msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> + hdr = genlmsg_put(msg, 0, 0, &dpll_nl_family, 0, event);
> + if (!hdr)
> + goto err_free_msg;
ret is uninitialised here, but it is used in the error path.
This is flagged by a clang-16 W=1 build, and Smatch.
> + ret = dpll_device_get_one(dpll, msg, NULL);
> + if (ret)
> + goto err_cancel_msg;
> + genlmsg_end(msg, hdr);
> + genlmsg_multicast(&dpll_nl_family, msg, 0, 0, GFP_KERNEL);
> +
> + return 0;
> +
> +err_cancel_msg:
> + genlmsg_cancel(msg, hdr);
> +err_free_msg:
> + nlmsg_free(msg);
> +
> + return ret;
> +}
...
> +int __dpll_device_change_ntf(struct dpll_device *dpll)
> +{
> + return dpll_device_event_send(DPLL_CMD_DEVICE_CHANGE_NTF, dpll);
> +}
Should this function be static?
...
> +static int
> +dpll_pin_event_send(enum dpll_cmd event, struct dpll_pin *pin)
> +{
> + struct sk_buff *msg;
> + void *hdr;
> + int ret;
> +
> + if (WARN_ON(!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED)))
> + return -ENODEV;
> +
> + msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
> + hdr = genlmsg_put(msg, 0, 0, &dpll_nl_family, 0, event);
> + if (!hdr)
> + goto err_free_msg;
It looks like ret is used uninitialised here too.
> + ret = dpll_cmd_pin_get_one(msg, pin, NULL);
> + if (ret)
> + goto err_cancel_msg;
> + genlmsg_end(msg, hdr);
> + genlmsg_multicast(&dpll_nl_family, msg, 0, 0, GFP_KERNEL);
> +
> + return 0;
> +
> +err_cancel_msg:
> + genlmsg_cancel(msg, hdr);
> +err_free_msg:
> + nlmsg_free(msg);
> +
> + return ret;
> +}
...
> +void
> +dpll_unlock_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
> + struct genl_info *info)
The indentation of the line above is not correct.
There are two spaces too many.
...
> +void dpll_netlink_finish(void)
> +{
> + genl_unregister_family(&dpll_nl_family);
> +}
Should this function be static?
...
Powered by blists - more mailing lists