netdev - Re: [PATCH net-next V3 2/2] rtnl: Add support for netdev event attribute to link messages

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]

Date:   Thu, 27 Apr 2017 17:11:54 -0700
From:   Roopa Prabhu <roopa@...ulusnetworks.com>
To:     Vlad Yasevich <vyasevic@...hat.com>
Cc:     David Ahern <dsa@...ulusnetworks.com>,
        Vladislav Yasevich <vyasevich@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH net-next V3 2/2] rtnl: Add support for netdev event
 attribute to link messages

On Thu, Apr 27, 2017 at 12:51 PM, Vlad Yasevich <vyasevic@...hat.com> wrote:
> On 04/24/2017 11:14 AM, Roopa Prabhu wrote:
>> On Sun, Apr 23, 2017 at 6:07 PM, David Ahern <dsa@...ulusnetworks.com> wrote:
>>>
>>> On 4/21/17 11:31 AM, Vladislav Yasevich wrote:
>>>> @@ -1276,9 +1277,40 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
>>>>       return err;
>>>>  }
>>>>
>>>> +static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event)
>>>> +{
>>>> +     u32 rtnl_event;
>>>> +
>>>> +     switch (event) {
>>>> +     case NETDEV_REBOOT:
>>>> +             rtnl_event = IFLA_EVENT_REBOOT;
>>>> +             break;
>>>> +     case NETDEV_FEAT_CHANGE:
>>>> +             rtnl_event = IFLA_EVENT_FEAT_CHANGE;
>>>> +             break;
>>>> +     case NETDEV_BONDING_FAILOVER:
>>>> +             rtnl_event = IFLA_EVENT_BONDING_FAILOVER;
>>>> +             break;
>>>> +     case NETDEV_NOTIFY_PEERS:
>>>> +             rtnl_event = IFLA_EVENT_NOTIFY_PEERS;
>>>> +             break;
>>>> +     case NETDEV_RESEND_IGMP:
>>>> +             rtnl_event = IFLA_EVENT_RESEND_IGMP;
>>>> +             break;
>>>> +     case NETDEV_CHANGEINFODATA:
>>>> +             rtnl_event = IFLA_EVENT_CHANGE_INFO_DATA;
>>>> +             break;
>>>> +     default:
>>>> +             return 0;
>>>> +     }
>>>> +
>>>> +     return nla_put_u32(skb, IFLA_EVENT, rtnl_event);
>>>> +}
>>>> +
>>>
>>> I still have doubts about encoding kernel events into a uapi.
>>
>> agree. I don't see why user-space will need NETDEV_CHANGEINFODATA and
>> others david listed.
>>
>
> Well, I am not sure about CHANGEINFODATA as well, but I can see use
> cases for others.
>
>> My other concerns are, once we have this exposed to user-space and
>> user-space starts relying on it, it will need accurate information and
>> will expect to have this event information all the time.
>> IIUC, we cannot cover multiple events in a single notification and not
>> all link notifications will contain an IFLA_EVENT attribute.
>
> Uhm...  If the rtnetlink message was a result of an event, it will have
> an IFLA_EVENT.  If a message is something else, then it will not have
> an event.  That's the point.  Not all netlink attributes are in every
> netlink message.
>
>> In other
>> words, we will be telling user-space to not expect that the kernel
>> will send IFLA_EVENT every time.
>>
>
> No, we are telling the user that if it is interested in a specific event
> (let's say NOTIFY_PEERS or RESEND_IGMP), then it now can monitor netlink
> traffic for those events.
> As things stand right now, that's not possible.
>
> I've done this specifically for all events for which we currently generate
> a netlink message.
>
> The only concern I have is that if in the future we remove a certain netdev
> event, it may impact applications.  But we may be doing it right now as well,
> only silently, and the apps may have to find some ways to work around it.
>

ok, fair enough. it might be ok then....except for the specific
attributes that user-space may not be interested like CHANGEINFODATA.