netdev - 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] [day] [month] [year] [list]

Message-ID: <DM6PR11MB4657642B8DDA56A8EE5CE24D9B882@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Fri, 23 Aug 2024 21:39:48 +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 11:23 PM
>
>On Fri, 23 Aug 2024 at 20:31, Kubalewski, Arkadiusz
><arkadiusz.kubalewski@...el.com> wrote:
>>
>> >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
>
>I tried the changes locally to check it would work and ended up with
>the following patch. Can you verify that it fixes the issue you had?
>
>diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
>index d42c1d605969..311542a8aa24 100644
>--- a/tools/net/ynl/lib/ynl.py
>+++ b/tools/net/ynl/lib/ynl.py
>@@ -386,8 +386,10 @@ class NetlinkProtocol:
>     def _decode(self, nl_msg):
>         return nl_msg
>
>-    def decode(self, ynl, nl_msg, op):
>+    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()]
>         fixed_header_size = ynl._struct_size(op.fixed_header)
>         msg.raw_attrs = NlAttrs(msg.raw, fixed_header_size)
>         return msg
>@@ -921,8 +923,7 @@ class YnlFamily(SpecFamily):
>                     print("Netlink done while checking for ntf!?")
>                     continue
>
>-                op = self.rsp_by_value[nl_msg.cmd()]
>-                decoded = self.nlproto.decode(self, nl_msg, op)
>+                decoded = self.nlproto.decode(self, nl_msg, None,
>self.rsp_by_value)
>                 if decoded.cmd() not in self.async_msg_ids:
>                     print("Unexpected msg id done while checking for
>ntf", decoded)
>                     continue
>@@ -980,7 +981,7 @@ class YnlFamily(SpecFamily):
>                     if nl_msg.extack:
>                         self._decode_extack(req_msg, op, nl_msg.extack)
>                 else:
>-                    op = self.rsp_by_value[nl_msg.cmd()]
>+                    op = None
>                     req_flags = []
>
>                 if nl_msg.error:
>@@ -1004,7 +1005,7 @@ class YnlFamily(SpecFamily):
>                     done = len(reqs_by_seq) == 0
>                     break
>
>-                decoded = self.nlproto.decode(self, nl_msg, op)
>+                decoded = self.nlproto.decode(self, nl_msg, op,
>self.rsp_by_value)
>
>                 # Check if this is a reply to our request
>                 if nl_msg.nl_seq not in reqs_by_seq or decoded.cmd()
>!= op.rsp_value:

I can confirm the notification works with above changes, but there is third
call to decode which would also need an update?
It shows up on testing messages which return extack errors.
Beware line numbers, these can differ on my ynl.py as had it modified slightly.

  File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 1040, in do
    return self._op(method, vals, flags)
  File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 1034, in _op
    ret = self._ops(ops)
  File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 982, in _ops
    self._decode_extack(req_msg, op, nl_msg.extack)
  File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 801, in _decode_extack
    msg = self.nlproto.decode(self, NlMsg(request, 0, op.attr_set), op)
TypeError: decode() missing 1 required positional argument: 'ops_by_value'
error: 1

Thank you!
Arkadiusz