netdev - Re: [PATCH net-next v2 9/9] geneve: Implement rtnl changelink

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: <CALnjE+pCAbM=sVdfieV5PDhhu0xdt_9JZOzFY-mUq5Xx=n+vsw@mail.gmail.com>
Date:	Thu, 20 Aug 2015 13:33:21 -0700
From:	Pravin Shelar <pshelar@...ira.com>
To:	Jesse Gross <jesse@...ira.com>
Cc:	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 9/9] geneve: Implement rtnl changelink

On Wed, Aug 19, 2015 at 3:39 PM, Jesse Gross <jesse@...ira.com> wrote:
> On Wed, Aug 19, 2015 at 1:12 PM, Pravin Shelar <pshelar@...ira.com> wrote:
>> On Wed, Aug 19, 2015 at 12:40 PM, Jesse Gross <jesse@...ira.com> wrote:
>>> On Mon, Aug 17, 2015 at 2:11 PM, Pravin B Shelar <pshelar@...ira.com> wrote:
>>>> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
>>>> index e47cdd9..0d7fbef 100644
>>>> --- a/drivers/net/geneve.c
>>>> +++ b/drivers/net/geneve.c
>>>>         geneve->remote.sin_addr.s_addr = rem_addr;
>>>>         if (IN_MULTICAST(ntohl(geneve->remote.sin_addr.s_addr)))
>>>>                 return -EINVAL;
>>>>
>>>> +       u32_to_vni(vni, geneve->vni);
>>>>         list_for_each_entry(t, &gn->geneve_list, next) {
>>>>                 if (!memcmp(geneve->vni, t->vni, sizeof(t->vni)) &&
>>>>                     rem_addr == t->remote.sin_addr.s_addr &&
>>>
>>> I'm not sure that these types of operations are safe if the device is
>>> already running. We first overwrite the remote value and then we do
>>> error checking but that means that if there is an error, then the
>>> device will be left in a broken state. Don't we also need to update
>>> the hash table if some of these parameters change?
>>>
>> ok, I will stop device before making changes. that way we can add it
>> to hash table.
>
> I think we should still strive to make changes as minimally disruptive
> as possible. At least some changes can still be done safely at runtime
> so it would be nice to be able to handle those cleanly.
>
>>>> +static int geneve_changelink(struct net_device *dev,
>>>> +                            struct nlattr *tb[], struct nlattr *data[])
>>>> +{
>>> [...]
>>>> -       if (data[IFLA_GENEVE_PORT])
>>>> -               dst_port = nla_get_u16(data[IFLA_GENEVE_PORT]);
>>>> +       if (geneve->sock && (dst_port != ntohs(geneve->dst_port) ||
>>>> +                            metadata != geneve->collect_md)) {
>>>
>>> It seems like in an ideal world, we wouldn't need to recreate the
>>> socket if metadata collection changed (assuming that there are no new
>>> conflicts).
>>
>> To keep changelink simple I am thinking of disallowing metadata changes.
>
> I guess I would rather allow it but make changes slower than disallow
> it. That way it is easier to improve in the future if necessary.

These changes need more refactoring in Geneve code. I will post it as
separate patch series once this series is in.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html