| 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: Fri, 24 Mar 2023 10:29:01 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Vadim Fedorenko <vadfed@...a.com>
Cc: Jakub Kicinski <kuba@...nel.org>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
Jonathan Lemon <jonathan.lemon@...il.com>,
Paolo Abeni <pabeni@...hat.com>,
Vadim Fedorenko <vadim.fedorenko@...ux.dev>, poros@...hat.com,
mschmidt@...hat.com, netdev@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
Milena Olech <milena.olech@...el.com>,
Michal Michalik <michal.michalik@...el.com>
Subject: Re: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions
Sun, Mar 12, 2023 at 03:28:03AM CET, vadfed@...a.com wrote:
[...]
>+static int
>+dpll_msg_add_pin_direction(struct sk_buff *msg, const struct dpll_pin *pin,
>+ struct netlink_ext_ack *extack)
>+{
>+ enum dpll_pin_direction direction;
>+ struct dpll_pin_ref *ref;
>+ unsigned long i;
>+
>+ xa_for_each((struct xarray *)&pin->dpll_refs, i, ref) {
>+ if (ref && ref->ops && ref->dpll)
>+ break;
>+ }
>+ if (!ref || !ref->ops || !ref->dpll)
>+ return -ENODEV;
>+ if (!ref->ops->direction_get)
>+ return -EOPNOTSUPP;
>+ if (ref->ops->direction_get(pin, ref->dpll, &direction, extack))
>+ return -EFAULT;
>+ if (nla_put_u8(msg, DPLL_A_PIN_DIRECTION, direction))
>+ return -EMSGSIZE;
>+
>+ return 0;
>+}
>+
>+static int
>+dpll_msg_add_pin_freq(struct sk_buff *msg, const struct dpll_pin *pin,
>+ struct netlink_ext_ack *extack, bool dump_any_freq)
>+{
>+ enum dpll_pin_freq_supp fs;
>+ struct dpll_pin_ref *ref;
>+ unsigned long i;
>+ u32 freq;
>+
>+ xa_for_each((struct xarray *)&pin->dpll_refs, i, ref) {
>+ if (ref && ref->ops && ref->dpll)
Checking for "ref" is nonsense here, as xa_for_each fills it up
for every iteration.
ref->dpll is always filled. Also pointless check.
Does it make sense to register with ops==NULL? I think we should
forbid it and make this just xa_find(0) to get the first item in the
xarray.
I'm doing this in my patch, as it is dependency on some other patch I do
in this area.
>+ break;
>+ }
>+ if (!ref || !ref->ops || !ref->dpll)
>+ return -ENODEV;
>+ if (!ref->ops->frequency_get)
>+ return -EOPNOTSUPP;
>+ if (ref->ops->frequency_get(pin, ref->dpll, &freq, extack))
>+ return -EFAULT;
>+ if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY, freq))
>+ return -EMSGSIZE;
>+ if (!dump_any_freq)
>+ return 0;
>+ for (fs = DPLL_PIN_FREQ_SUPP_UNSPEC + 1;
>+ fs <= DPLL_PIN_FREQ_SUPP_MAX; fs++) {
>+ if (test_bit(fs, &pin->prop.freq_supported)) {
>+ if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED,
>+ dpll_pin_freq_value[fs]))
>+ return -EMSGSIZE;
>+ }
>+ }
>+ if (pin->prop.any_freq_min && pin->prop.any_freq_max) {
>+ if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MIN,
>+ pin->prop.any_freq_min))
>+ return -EMSGSIZE;
>+ if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MAX,
>+ pin->prop.any_freq_max))
>+ return -EMSGSIZE;
>+ }
>+
>+ return 0;
>+}
>+
[...]
>+static int
>+dpll_cmd_pin_on_dpll_get(struct sk_buff *msg, struct dpll_pin *pin,
>+ struct dpll_device *dpll,
>+ struct netlink_ext_ack *extack)
>+{
>+ struct dpll_pin_ref *ref;
>+ int ret;
>+
>+ if (nla_put_u32(msg, DPLL_A_PIN_IDX, pin->dev_driver_id))
>+ return -EMSGSIZE;
>+ if (nla_put_string(msg, DPLL_A_PIN_DESCRIPTION, pin->prop.description))
>+ return -EMSGSIZE;
>+ if (nla_put_u8(msg, DPLL_A_PIN_TYPE, pin->prop.type))
>+ return -EMSGSIZE;
>+ if (nla_put_u32(msg, DPLL_A_PIN_DPLL_CAPS, pin->prop.capabilities))
>+ return -EMSGSIZE;
>+ ret = dpll_msg_add_pin_direction(msg, pin, extack);
>+ if (ret)
Why -EOPNOTSUPP here is not ok, as for the others below?
>+ return ret;
>+ ret = dpll_msg_add_pin_freq(msg, pin, extack, true);
>+ if (ret && ret != -EOPNOTSUPP)
>+ return ret;
>+ ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll);
>+ if (!ref)
How this can happen? I don't think it could.
>+ return -EFAULT;
>+ ret = dpll_msg_add_pin_prio(msg, pin, ref, extack);
>+ if (ret && ret != -EOPNOTSUPP)
>+ return ret;
>+ ret = dpll_msg_add_pin_on_dpll_state(msg, pin, ref, extack);
>+ if (ret && ret != -EOPNOTSUPP)
>+ return ret;
>+ ret = dpll_msg_add_pin_parents(msg, pin, extack);
>+ if (ret)
>+ return ret;
>+ if (pin->rclk_dev_name)
>+ if (nla_put_string(msg, DPLL_A_PIN_RCLK_DEVICE,
>+ pin->rclk_dev_name))
>+ return -EMSGSIZE;
>+
>+ return 0;
>+}
>+
>+static int
>+__dpll_cmd_pin_dump_one(struct sk_buff *msg, struct dpll_pin *pin,
>+ struct netlink_ext_ack *extack, bool dump_dpll)
>+{
>+ int ret;
>+
>+ if (nla_put_u32(msg, DPLL_A_PIN_IDX, pin->dev_driver_id))
>+ return -EMSGSIZE;
>+ if (nla_put_string(msg, DPLL_A_PIN_DESCRIPTION, pin->prop.description))
>+ return -EMSGSIZE;
>+ if (nla_put_u8(msg, DPLL_A_PIN_TYPE, pin->prop.type))
>+ return -EMSGSIZE;
>+ ret = dpll_msg_add_pin_direction(msg, pin, extack);
>+ if (ret)
>+ return ret;
>+ ret = dpll_msg_add_pin_freq(msg, pin, extack, true);
>+ if (ret && ret != -EOPNOTSUPP)
>+ return ret;
>+ ret = dpll_msg_add_pins_on_pin(msg, pin, extack);
>+ if (ret)
>+ return ret;
>+ if (!xa_empty(&pin->dpll_refs) && dump_dpll) {
How dpll refs could be empty? I don't think it is possible.
Overall, whole the code has very odd habit of checking for conditions
that are obviously impossible to happen. Only confuses reader as he
naturally expects that the check is there for a reason.
>+ ret = dpll_msg_add_pin_dplls(msg, pin, extack);
>+ if (ret)
>+ return ret;
>+ }
>+ if (pin->rclk_dev_name)
>+ if (nla_put_string(msg, DPLL_A_PIN_RCLK_DEVICE,
>+ pin->rclk_dev_name))
>+ return -EMSGSIZE;
>+
>+ return 0;
>+}
>+
Powered by blists - more mailing lists