linux-kernel - RE: [RFC PATCH net] tools/net/ynl: fix cli.py --subscribe feature

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: <DM6PR11MB46578FDE21C9E875A7F3C1BE9B882@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Fri, 23 Aug 2024 19:31:07 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Donald Hunter <donald.hunter@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "kuba@...nel.org"
	<kuba@...nel.org>, "davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "pabeni@...hat.com"
	<pabeni@...hat.com>, "jiri@...nulli.us" <jiri@...nulli.us>, "Keller, Jacob E"
	<jacob.e.keller@...el.com>, "liuhangbin@...il.com" <liuhangbin@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC PATCH net] tools/net/ynl: fix cli.py --subscribe feature

>From: Donald Hunter <donald.hunter@...il.com>
>Sent: Friday, August 23, 2024 12:40 PM
>
>Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com> writes:
>
>> Execution of command:
>> ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml /
>> 	--subscribe "monitor" --sleep 10
>> fails with:
>> Traceback (most recent call last):
>>   File "/root/arek/linux-dpll/./tools/net/ynl/cli.py", line 114, in <module>
>>     main()
>>   File "/root/arek/linux-dpll/./tools/net/ynl/cli.py", line 109, in main
>>     ynl.check_ntf()
>>   File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 924, in
>>check_ntf
>>     op = self.rsp_by_value[nl_msg.cmd()]
>> KeyError: 19
>>
>> The key value of 19 returned from nl_msg.cmd() is a received message
>> header's nl_type, which is the id value of generic netlink family being
>> addressed in the OS on subscribing. It is wrong to use it for decoding
>> the notification. Expected notification message on dpll subsystem is
>> DPLL_CMD_PIN_CHANGE_NTF=13, seems at that point only available as first
>> byte of RAW message payload, use it to target correct op and allow further
>> parsing.
>>
>> Fixes: "0a966d606c68" ("tools/net/ynl: Fix extack decoding for directional
>>ops")
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>> ---
>>  tools/net/ynl/lib/ynl.py | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
>> index d42c1d605969..192d6c150303 100644
>> --- a/tools/net/ynl/lib/ynl.py
>> +++ b/tools/net/ynl/lib/ynl.py
>> @@ -921,7 +921,7 @@ class YnlFamily(SpecFamily):
>>                      print("Netlink done while checking for ntf!?")
>>                      continue
>>
>> -                op = self.rsp_by_value[nl_msg.cmd()]
>> +                op = self.rsp_by_value[nl_msg.raw[0]]
>
>I don't think that is the right fix. It would break notifications for
>raw netlink messages. The point of NlMsg.cmd() is to abstract away where
>the op id comes from. GenlMsg.cmd() returns the value unpacked from
>raw[0].
>

Well, me either, thus the RFC. Your suggestion makes much more sense than
my workaround.

>The problem is that we are trying to look up the op before calling
>nlproto.decode(...) but it wants to know the op to check if it has a
>fixed header.
>
>I think the fix would be to change NetlinkProtocol.decode() to perform
>the op lookup, if necessary, after it has called self._decode() to
>unpack the GenlMsg.
>
>How about changing NetlinkProtocol.decode() to be:
>
>def decode(self, ynl, nl_msg, op, ops_by_value):
>    msg = self._decode(nl_msg)
>    if op is None:
>        op = ops_by_value[msg.cmd()]
>    ...
>
>The main loop can call it like this:
>
>nlproto.decode(self, nl_msg, op, self.rsp_by_value)
>
>and check_ntf() can call it like this:
>
>nlproto.decode(self, nl_msg, None, self.rsp_by_value)
>

Yes, again, this seems much better, I will prepare new patch and send
the non-RFC version soon.

Thanks for your help!
Arkadiusz

>>                  decoded = self.nlproto.decode(self, nl_msg, op)
>>                  if decoded.cmd() not in self.async_msg_ids:
>>                      print("Unexpected msg id done while checking for ntf",
>>decoded)