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: <DM6PR11MB46574E6BDA6B6F5E8A6E9AB69B682@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Thu, 11 Jan 2024 09:06:55 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"vadim.fedorenko@...ux.dev" <vadim.fedorenko@...ux.dev>, "Olech, Milena"
	<milena.olech@...el.com>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "Glaza, Jan" <jan.glaza@...el.com>
Subject: RE: [PATCH net v2 3/4] dpll: fix register pin with unregistered
 parent pin

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Thursday, January 4, 2024 3:48 PM
>
>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?
>

Yes, perfect sense. Will do.

Thank you!
Arkadiusz

>
>
>>
>> 	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
>>