[v5,0/2] Make neighbor eviction controllable by userspace

Message

James Prestwood Oct. 21, 2021, 12:32 a.m. UTC

v1 -> v2:

 - It was suggested by Daniel Borkmann to extend the neighbor table settings
   rather than adding IPv4/IPv6 options for ARP/NDISC separately. I agree
   this way is much more concise since there is now only one place where the
   option is checked and defined.
 - Moved documentation/code into the same patch
 - Explained in more detail the test scenario and results

v2 -> v3:

 - Renamed 'skip_perm' to 'nocarrier'. The way this parameter is used
   matches this naming.
 - Changed logic to still flush if 'nocarrier' is false.

v3 -> v4:

 - Moved NDTPA_EVICT_NOCARRIER after NDTPA_PAD

v4 -> v5:

 - Went back to the original v1 patchset and changed:
 - Used ANDCONF for IN_DEV macro
 - Got RCU lock prior to __in_dev_get_rcu(). Do note that the logic
   here was extended to handle if __in_dev_get_rcu() fails. If this
   happens the existing behavior should be maintained and set the
   carrier down. I'm unsure if get_rcu() can fail in this context
   though. Similar logic was used for in6_dev_get.
 - Changed ndisc_evict_nocarrier to use a u8, proper handler, and
   set min/max values.


Cc: David S. Miller <[e-mail]>
Cc: Jakub Kicinski <[e-mail]>
Cc: Jonathan Corbet <[e-mail]>
Cc: Hideaki YOSHIFUJI <[e-mail]>
Cc: David Ahern <[e-mail]>
Cc: Roopa Prabhu <[e-mail]>
Cc: Daniel Borkmann <[e-mail]>
Cc: Vladimir Oltean <[e-mail]>
Cc: Ido Schimmel <[e-mail]>
Cc: Nikolay Aleksandrov <[e-mail]>
Cc: Chinmay Agarwal <[e-mail]>
Cc: Yajun Deng <[e-mail]>
Cc: Tong Zhu <[e-mail]>
Cc: Johannes Berg <[e-mail]>
Cc: Jouni Malinen <[e-mail]>

James Prestwood (2):
  net: arp: introduce arp_evict_nocarrier sysctl parameter
  net: ndisc: introduce ndisc_evict_nocarrier sysctl parameter

 Documentation/networking/ip-sysctl.rst | 18 ++++++++++++++++++
 include/linux/inetdevice.h             |  2 ++
 include/linux/ipv6.h                   |  1 +
 include/uapi/linux/ip.h                |  1 +
 include/uapi/linux/ipv6.h              |  1 +
 include/uapi/linux/sysctl.h            |  1 +
 net/ipv4/arp.c                         | 13 ++++++++++++-
 net/ipv4/devinet.c                     |  4 ++++
 net/ipv6/addrconf.c                    | 12 ++++++++++++
 net/ipv6/ndisc.c                       |  5 ++++-
 10 files changed, 56 insertions(+), 2 deletions(-)

Comments

David Ahern Oct. 21, 2021, 2:33 a.m. UTC | #1

On 10/20/21 6:32 PM, James Prestwood wrote:
> v1 -> v2:
> 
>  - It was suggested by Daniel Borkmann to extend the neighbor table settings
>    rather than adding IPv4/IPv6 options for ARP/NDISC separately. I agree
>    this way is much more concise since there is now only one place where the
>    option is checked and defined.
>  - Moved documentation/code into the same patch
>  - Explained in more detail the test scenario and results
> 
> v2 -> v3:
> 
>  - Renamed 'skip_perm' to 'nocarrier'. The way this parameter is used
>    matches this naming.
>  - Changed logic to still flush if 'nocarrier' is false.
> 
> v3 -> v4:
> 
>  - Moved NDTPA_EVICT_NOCARRIER after NDTPA_PAD
> 
> v4 -> v5:
> 
>  - Went back to the original v1 patchset and changed:
>  - Used ANDCONF for IN_DEV macro
>  - Got RCU lock prior to __in_dev_get_rcu(). Do note that the logic
>    here was extended to handle if __in_dev_get_rcu() fails. If this
>    happens the existing behavior should be maintained and set the
>    carrier down. I'm unsure if get_rcu() can fail in this context
>    though. Similar logic was used for in6_dev_get.
>  - Changed ndisc_evict_nocarrier to use a u8, proper handler, and
>    set min/max values.
> 

I'll take a deep dive on the patches tomorrow.

You need to add a selftests script under tools/testing/selftests/net
that shows this behavior with the new setting set and unset. This is
easily done with veth pairs and network namespaces (one end of the veth
pair down sets the other into no-carrier). Take a look at the scripts
there - e.g., fib_nexthops.sh should provide a template for a start point.

James Prestwood Oct. 21, 2021, 9:59 p.m. UTC | #2

Hi David,

On Wed, 2021-10-20 at 20:33 -0600, David Ahern wrote:
> On 10/20/21 6:32 PM, James Prestwood wrote:
> > v1 -> v2:
> > 
> >  - It was suggested by Daniel Borkmann to extend the neighbor table
> > settings
> >    rather than adding IPv4/IPv6 options for ARP/NDISC separately. I
> > agree
> >    this way is much more concise since there is now only one place
> > where the
> >    option is checked and defined.
> >  - Moved documentation/code into the same patch
> >  - Explained in more detail the test scenario and results
> > 
> > v2 -> v3:
> > 
> >  - Renamed 'skip_perm' to 'nocarrier'. The way this parameter is
> > used
> >    matches this naming.
> >  - Changed logic to still flush if 'nocarrier' is false.
> > 
> > v3 -> v4:
> > 
> >  - Moved NDTPA_EVICT_NOCARRIER after NDTPA_PAD
> > 
> > v4 -> v5:
> > 
> >  - Went back to the original v1 patchset and changed:
> >  - Used ANDCONF for IN_DEV macro
> >  - Got RCU lock prior to __in_dev_get_rcu(). Do note that the logic
> >    here was extended to handle if __in_dev_get_rcu() fails. If this
> >    happens the existing behavior should be maintained and set the
> >    carrier down. I'm unsure if get_rcu() can fail in this context
> >    though. Similar logic was used for in6_dev_get.
> >  - Changed ndisc_evict_nocarrier to use a u8, proper handler, and
> >    set min/max values.
> > 
> 
> I'll take a deep dive on the patches tomorrow.
> 
> You need to add a selftests script under tools/testing/selftests/net
> that shows this behavior with the new setting set and unset. This is
> easily done with veth pairs and network namespaces (one end of the
> veth
> pair down sets the other into no-carrier). Take a look at the scripts
> there - e.g., fib_nexthops.sh should provide a template for a start
> point.

So the test itself is pretty simple. The part I'm unsure about is how
you actually set the carrier state from userspace. I see "ip link set
<dev> carrier {on,off}" but this reports not supported for veths,
wlans, and eth interfaces I have tried. AFAIK the driver controls the
carrier state. Maybe some drivers do support this?

Is there a way to set the carrier state that you, or anyone is aware
of?

Thanks,
James

David Ahern Oct. 21, 2021, 10:09 p.m. UTC | #3

On 10/21/21 3:59 PM, James Prestwood wrote:
> So the test itself is pretty simple. The part I'm unsure about is how
> you actually set the carrier state from userspace. I see "ip link set
> <dev> carrier {on,off}" but this reports not supported for veths,
> wlans, and eth interfaces I have tried. AFAIK the driver controls the
> carrier state. Maybe some drivers do support this?
> 
> Is there a way to set the carrier state that you, or anyone is aware
> of?

with veth pairs, if you set one down the other goes into no-carrier mode

make veth0 the one you are validating and put veth1 in a namespace
called testing. Then 'ip -netns testing li set veth1 down' put veth0 in
NO_CARRIER