netdev - Re: [PATCH net] net: openvswitch: fix TTL decrement action netlink message format

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: <7259C596-A195-4E3B-9D6F-959A34FCC3E2@redhat.com>
Date:   Tue, 24 Nov 2020 12:19:52 +0100
From:   "Eelco Chaudron" <echaudro@...hat.com>
To:     "Matteo Croce" <mcroce@...ux.microsoft.com>
Cc:     "Jakub Kicinski" <kuba@...nel.org>, netdev@...r.kernel.org,
        "David Miller" <davem@...emloft.net>, dev@...nvswitch.org,
        "Pravin B Shelar" <pshelar@....org>, bindiyakurle@...il.com,
        "Ilya Maximets" <i.maximets@....org>
Subject: Re: [PATCH net] net: openvswitch: fix TTL decrement action netlink
 message format



On 23 Nov 2020, at 20:36, Matteo Croce wrote:

> On Fri, Nov 20, 2020 at 10:12 PM Jakub Kicinski <kuba@...nel.org> 
> wrote:
>>
>> On Thu, 19 Nov 2020 04:04:04 -0500 Eelco Chaudron wrote:
>>> Currently, the openvswitch module is not accepting the correctly 
>>> formated
>>> netlink message for the TTL decrement action. For both setting and 
>>> getting
>>> the dec_ttl action, the actions should be nested in the
>>> OVS_DEC_TTL_ATTR_ACTION attribute as mentioned in the openvswitch.h 
>>> uapi.
>>
>> IOW this change will not break any known user space, correct?
>>
>> But existing OvS user space already expects it to work like you
>> make it work now?
>>
>> What's the harm in leaving it as is?
>>
>>> Fixes: 744676e77720 ("openvswitch: add TTL decrement action")
>>> Signed-off-by: Eelco Chaudron <echaudro@...hat.com>
>>
>> Can we get a review from OvS folks? Matteo looks good to you (as the
>> original author)?
>>
>
> Hi,
>
> I think that the userspace still has to implement the dec_ttl action;
> by now dec_ttl is implemented with set_ttl().
> So there is no breakage yet.

Yes, see reply to Jakub’s email.

> Eelco, with this fix we will encode the netlink attribute in the same
> way for the kernel and netdev datapath?

Yes, this should make both implementations the same. No more weird code 
in the data-plane agnostic code :)

> If so, go for it.
>
>
>>> -     err = __ovs_nla_copy_actions(net, attr, key, sfa, eth_type,
>>> +     err = __ovs_nla_copy_actions(net, actions, key, sfa, eth_type,
>>>                                    vlan_tci, mpls_label_count, log);
>>>       if (err)
>>>               return err;
>>
>> You're not canceling any nests on error, I assume this is normal.
>>
>>> +     add_nested_action_end(*sfa, action_start);
>>>       add_nested_action_end(*sfa, start);
>>>       return 0;
>>>  }
>>
>
>
> -- 
> per aspera ad upstream