| 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
| ||
|
Message-ID: <ZBmyfzr/VLsLVp7Y@nanopsycho>
Date: Tue, 21 Mar 2023 14:34:55 +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_event_device_change(struct sk_buff *msg, struct dpll_device *dpll,
>+ struct dpll_pin *pin, struct dpll_pin *parent,
>+ enum dplla attr)
>+{
>+ int ret = dpll_msg_add_dev_handle(msg, dpll);
>+ struct dpll_pin_ref *ref = NULL;
>+ enum dpll_pin_state state;
>+
>+ if (ret)
>+ return ret;
>+ if (pin && nla_put_u32(msg, DPLL_A_PIN_IDX, pin->dev_driver_id))
>+ return -EMSGSIZE;
>+
>+ switch (attr) {
>+ case DPLL_A_MODE:
>+ ret = dpll_msg_add_mode(msg, dpll, NULL);
>+ break;
>+ case DPLL_A_SOURCE_PIN_IDX:
>+ ret = dpll_msg_add_source_pin_idx(msg, dpll, NULL);
>+ break;
>+ case DPLL_A_LOCK_STATUS:
>+ ret = dpll_msg_add_lock_status(msg, dpll, NULL);
On top of what I wrote about the notifications, I found another two
issues:
1) You don't take any lock calling this from drivers. You need to hold
the xarray locks you have now.
I have to repear, I think that we definitelly need to convert the
overall locking scheme to have this per-instance, in a similar way
we did that for devlink. I noted this in another email, but wanted
to say that again.
2) You have possible race condition:
1) -> driver gets a state change event
2) -> driver calls into this function
3) -> this code does call the driver op to get the state, driver
queries the state again
Between 1) and 3) state can easily change, multiple times. That might
lead to oddities observed by the user (like getting a notification
of change with the original values)
I see only 1 solutions to this:
Pass the value of changed item from the driver here and just pass
it on over netlink without doing calling into driver again.
>+ break;
>+ case DPLL_A_TEMP:
>+ ret = dpll_msg_add_temp(msg, dpll, NULL);
>+ break;
>+ case DPLL_A_PIN_FREQUENCY:
>+ ret = dpll_msg_add_pin_freq(msg, pin, NULL, false);
>+ break;
>+ case DPLL_A_PIN_PRIO:
>+ ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll);
>+ if (!ref)
>+ return -EFAULT;
>+ ret = dpll_msg_add_pin_prio(msg, pin, ref, NULL);
>+ break;
>+ case DPLL_A_PIN_STATE:
>+ if (parent) {
>+ ref = dpll_xa_ref_pin_find(&pin->parent_refs, parent);
>+ if (!ref)
>+ return -EFAULT;
>+ if (!ref->ops || !ref->ops->state_on_pin_get)
>+ return -EOPNOTSUPP;
>+ ret = ref->ops->state_on_pin_get(pin, parent, &state,
>+ NULL);
>+ if (ret)
>+ return ret;
>+ if (nla_put_u32(msg, DPLL_A_PIN_PARENT_IDX,
>+ parent->dev_driver_id))
>+ return -EMSGSIZE;
>+ } else {
>+ ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll);
>+ if (!ref)
>+ return -EFAULT;
>+ ret = dpll_msg_add_pin_on_dpll_state(msg, pin, ref,
>+ NULL);
>+ if (ret)
>+ return ret;
>+ }
>+ break;
>+ default:
>+ break;
>+ }
>+
>+ return ret;
>+}
>+
>+static int
>+dpll_send_event_create(enum dpll_event event, struct dpll_device *dpll)
>+{
>+ struct sk_buff *msg;
>+ int ret = -EMSGSIZE;
>+ void *hdr;
>+
>+ 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 out_free_msg;
>+
>+ ret = dpll_msg_add_dev_handle(msg, dpll);
>+ if (ret)
>+ goto out_cancel_msg;
>+ genlmsg_end(msg, hdr);
>+ genlmsg_multicast(&dpll_nl_family, msg, 0, 0, GFP_KERNEL);
>+
>+ return 0;
>+
>+out_cancel_msg:
>+ genlmsg_cancel(msg, hdr);
>+out_free_msg:
>+ nlmsg_free(msg);
>+
>+ return ret;
>+}
>+
>+static int
>+dpll_send_event_change(struct dpll_device *dpll, struct dpll_pin *pin,
>+ struct dpll_pin *parent, enum dplla attr)
>+{
>+ struct sk_buff *msg;
>+ int ret = -EMSGSIZE;
>+ void *hdr;
>+
>+ msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>+ if (!msg)
>+ return -ENOMEM;
>+
>+ hdr = genlmsg_put(msg, 0, 0, &dpll_nl_family, 0,
>+ DPLL_EVENT_DEVICE_CHANGE);
>+ if (!hdr)
>+ goto out_free_msg;
>+
>+ ret = dpll_event_device_change(msg, dpll, pin, parent, attr);
>+ if (ret)
>+ goto out_cancel_msg;
>+ genlmsg_end(msg, hdr);
>+ genlmsg_multicast(&dpll_nl_family, msg, 0, 0, GFP_KERNEL);
>+
>+ return 0;
>+
>+out_cancel_msg:
>+ genlmsg_cancel(msg, hdr);
>+out_free_msg:
>+ nlmsg_free(msg);
>+
>+ return ret;
>+}
>+
>+int dpll_notify_device_create(struct dpll_device *dpll)
>+{
>+ return dpll_send_event_create(DPLL_EVENT_DEVICE_CREATE, dpll);
>+}
>+
>+int dpll_notify_device_delete(struct dpll_device *dpll)
Please change the function names to "register/unregister" to be
consistent with the rest of the code.
>+{
>+ return dpll_send_event_create(DPLL_EVENT_DEVICE_DELETE, dpll);
>+}
>+
>+int dpll_device_notify(struct dpll_device *dpll, enum dplla attr)
>+{
>+ if (WARN_ON(!dpll))
>+ return -EINVAL;
>+
>+ return dpll_send_event_change(dpll, NULL, NULL, attr);
>+}
>+EXPORT_SYMBOL_GPL(dpll_device_notify);
>+
>+int dpll_pin_notify(struct dpll_device *dpll, struct dpll_pin *pin,
>+ enum dplla attr)
The driver should be aware of netlink attributes. Should be
abstracted out.
just have per-item notification like:
dpll_pin_state_notify()
dpll_pin_prio_notify()
...
Then you can easily pass changed value that would allow solution to
the issue 2) I described above.
>+{
>+ return dpll_send_event_change(dpll, pin, NULL, attr);
>+}
>+
[...]
Powered by blists - more mailing lists