netdev - Re: [PATCH net v2 3/4] dpll: fix register pin with unregistered parent pin

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZZbFNMMiRvgSi1Ge@nanopsycho>
Date: Thu, 4 Jan 2024 15:48:20 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
Cc: netdev@...r.kernel.org, vadim.fedorenko@...ux.dev,
	michal.michalik@...el.com, milena.olech@...el.com,
	pabeni@...hat.com, kuba@...nel.org, Jan Glaza <jan.glaza@...el.com>
Subject: Re: [PATCH net v2 3/4] dpll: fix register pin with unregistered
 parent pin

Thu, Jan 04, 2024 at 12:11:31PM CET, arkadiusz.kubalewski@...el.com wrote:
>In case of multiple kernel module instances using the same dpll device:
>if only one registers dpll device, then only that one can register
>directly connected pins with a dpll device. When unregistered parent is
>responsible for determining if the muxed pin can be registered with it
>or not, the drivers need to be loaded in serialized order to work
>correctly - first the driver instance which registers the direct pins
>needs to be loaded, then the other instances could register muxed type
>pins.
>
>Allow registration of a pin with a parent even if the parent was not
>yet registered, thus allow ability for unserialized driver instance
>load order.
>Do not WARN_ON notification for unregistered pin, which can be invoked
>for described case, instead just return error.
>
>Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
>Reviewed-by: Jan Glaza <jan.glaza@...el.com>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>---
> drivers/dpll/dpll_core.c    | 4 ----
> drivers/dpll/dpll_netlink.c | 2 +-
> 2 files changed, 1 insertion(+), 5 deletions(-)
>
>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>index 0b469096ef79..c8a2129f5699 100644
>--- a/drivers/dpll/dpll_core.c
>+++ b/drivers/dpll/dpll_core.c
>@@ -28,8 +28,6 @@ static u32 dpll_xa_id;
> 	WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
> #define ASSERT_DPLL_NOT_REGISTERED(d)	\
> 	WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>-#define ASSERT_PIN_REGISTERED(p)	\
>-	WARN_ON_ONCE(!xa_get_mark(&dpll_pin_xa, (p)->id, DPLL_REGISTERED))
> 
> struct dpll_device_registration {
> 	struct list_head list;
>@@ -664,8 +662,6 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
> 	    WARN_ON(!ops->state_on_pin_get) ||
> 	    WARN_ON(!ops->direction_get))
> 		return -EINVAL;
>-	if (ASSERT_PIN_REGISTERED(parent))
>-		return -EINVAL;

This makes the pin-on-device and pin-on-pin register behaviour
different:
int
dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin,
                  const struct dpll_pin_ops *ops, void *priv)
{
	...
        if (ASSERT_DPLL_REGISTERED(dpll))
                return -EINVAL;

I think it is need to maintain the same set of restrictions and
behaviour the same for both.

With what you suggest, the user would just see couple of pins with no
parent (hidden one), no dpll devices (none would be registered).
That's odd.

PF0 is the owner of DPLL in your case.

>From the user perspective, I think it should look like:
1) If PFn appears after PF0, it registers pins related to it, PF0
   created instances are there and valid. User sees them all.
2) If PF0 gets removed before PFn, it removes all dpll related entities,
   even those related to PFn. Users sees nothing.

So you have to make sure that the pin is hidden (not shown to the user)
in case the parent (device/pin) is not registered. Makes sense?



> 
> 	mutex_lock(&dpll_lock);
> 	ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv);
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index 3ce9995013f1..f266db8da2f0 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -553,7 +553,7 @@ dpll_pin_event_send(enum dpll_cmd event, struct dpll_pin *pin)
> 	int ret = -ENOMEM;
> 	void *hdr;
> 
>-	if (WARN_ON(!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED)))
>+	if (!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED))
> 		return -ENODEV;
> 
> 	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>-- 
>2.38.1
>