netdev - Re: [PATCH] Ipvlan should return an error when an address is already in use.

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: <20170104100425.GE3009@templeofstupid.com>
Date:   Wed, 4 Jan 2017 02:04:25 -0800
From:   Krister Johansen <kjlx@...pleofstupid.com>
To:     David Miller <davem@...emloft.net>
Cc:     kjlx@...pleofstupid.com, maheshb@...gle.com, netdev@...r.kernel.org
Subject: Re: [PATCH] Ipvlan should return an error when an address is already
 in use.

On Sun, Jan 01, 2017 at 10:26:32PM -0500, David Miller wrote:
> From: Krister Johansen <kjlx@...pleofstupid.com>
> Date: Fri, 30 Dec 2016 20:10:58 -0800
> 
> > The ipvlan code already knows how to detect when a duplicate address is
> > about to be assigned to an ipvlan device.  However, that failure is not
> > propogated outward and leads to a silent failure.  This teaches the ip
> > address addition functions how to report this error to the user
> > applications so that a notifier chain failure during ip address addition
> > will not appear to succeed when it actually has not.
> > 
> > This can be especially useful if it is necessary to provision many
> > ipvlans in containers.  The provisioning software (or operator) can use
> > this to detect situations where an ip address is unexpectedly in use.
> > 
> > Signed-off-by: Krister Johansen <kjlx@...pleofstupid.com>
> 
> Your patch isn't handling the case of primary address promotions,
> which also issue NETDEV_UP events on these notifier chains.
> 
> But on a more basic level, it's extremely important that once you
> start using the notifier_{from,to}_errno() handling for a notifier,
> you must start doing so for all such cases of that notifier.

Thanks for taking a look.  I'm relatively new to this area of the code.
I do appreciate the feedback.

In terms of interpreting your final comment, does that mean any call on
the inetaddr_chain or inet6addr_chain, and if so would that include all
callers of call_netdevice_notifiers()?

Another concern that I had with my approach was that the
blocking_notifier_call_chain() occurs after a rtmsg_ifa(RTM_NEWADDR)
call.  In the places where I put rollback code, the address delete ends
up generating a corresponding rtmsg_ifa(RTM_DELADDR).  This is likely to
emit messages that make it look like the address was created and
deleted, except that the validation failed before creation was
completed.  Is the rtmsg_ifa() call consumed by other listeners in the
kernel, or is this purely a generation of a rtnetlink message for
applications listening in userland?

More generally, is using the notifier_call_chain a reasonable way of
getting this error information back to userland or would a different
approach be better?  I'm also concerned about the cases where the code
currently treats these invocations as atomic.  I had considered
introducing an alternate chain that might allow us to check that the
operation is valid prior to committing the change, but that seemed
potentially racy and that it might involve a lot of extra mechanism.

-K