Jakub Kicinski <[email protected]> writes: > On Tue, 15 Aug 2023 08:41:50 -0400 Aaron Conole wrote: >> > Validate the inputes. Now the second command correctly returns: >> >> s/inputes/inputs/ > > Thanks, fixed when applying > >> > $ ./cli.py --spec netlink/specs/ovs_vport.yaml \ >> > --do new \ >> > --json '{"upcall-pid": "00000001", "name": "some-port0", >> > "dp-ifindex":3,"ifindex":4294901760,"type":2}' >> > >> > lib.ynl.NlError: Netlink error: Numerical result out of range >> > nl_len = 108 (92) nl_flags = 0x300 nl_type = 2 >> > error: -34 extack: {'msg': 'integer out of range', 'unknown': >> > [[type:4 len:36] >> > b'\x0c\x00\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x0c\x00\x03\x00\xff\xff\xff\x7f\x00\x00\x00\x00\x08\x00\x01\x00\x08\x00\x00\x00'], >> > 'bad-attr': '.ifindex'} >> > >> > Accept 0 since it used to be silently ignored. >> > >> > Fixes: 54c4ef34c4b6 ("openvswitch: allow specifying ifindex of new >> > interfaces") >> > Reported-by: [email protected] >> > Signed-off-by: Jakub Kicinski <[email protected]> >> > --- >> > CC: [email protected] >> > CC: [email protected] >> > CC: [email protected] >> > CC: [email protected] >> > --- >> >> Thanks for the quick follow up. I accidentally broke my system trying >> to setup to reproduce the syzbot splat. > > Ah. Syzbot pointed at my commit so I thought others will just think > "not my bug" :) > >> The attribute here isn't used by the ovs-vswitchd, so probably why we >> never caught an issue before. I'll think about how to improve the >> fuzzing on the ovs module. At the very least, maybe we can have some >> additional checks in the netlink selftest. > > Speaking of fuzzing - reaching out to Dmitry crossed my mind. > When the first netlink specs got merged we briefly discussed > using them to guide syzbot a little. But then I thought - syzbot did > find this fairly quickly, it's more that previously we apparently had > no warning or crash for negative ifindex so there was no target to hit. > >> I noticed that since I copied the definitions when building >> ovs-dpctl.py, I have the same kind of mistake there (using unsigned for >> ifindex). I can submit a follow up to correct that definition. Also, >> we might consider correcting the yaml. > > FWIW I left the nla_put_u32() when outputting ifindex in the kernel as > well.
I looked around and it is a bit inconsistent when sending the ifindex. Some places are s32, some are u32, and doesn't seem to be any rhyme or reason other than "feels good here." > I needed s32 for the range because min and max are 16 bit (to > conserve space in the policy) so I could not express the positive limit > on u32. Whether ifindex is u32 or s32 is a bit of a philosophical > question to me, as it only takes positive 31b values... Yeah, we can always just use some number > INT_MAX, and we will have the sign bit as well, so it isn't too important. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
