Re: [PATCH net-next V8] net: Variable SLAAC: SLAAC with prefixes of arbitrary length in PIO

[Dmytro Shytyi]

Hello Jakub,

---- On Wed, 16 Dec 2020 18:28:31 +0100 Jakub Kicinski <kuba@xxxxxxxxxx> wrote ----

 > On Wed, 16 Dec 2020 15:01:33 +0100 Dmytro Shytyi wrote: 
 > > Hello David, 
 > > 
 > > Thank you for your comment. 
 > > Asnwers in-line. 
 > > 
 > > Take care, 
 > > 
 > > Dmytro SHYTYI 
 > > 
 > > 
 > > ---- On Wed, 16 Dec 2020 01:00:49 +0100 David Miller <davem@xxxxxxxxxxxxx> wrote ---- 
 > > 
 > >  > From: Dmytro Shytyi <dmytro@xxxxxxxxxx> 
 > >  > Date: Wed, 09 Dec 2020 04:27:54 +0100 
 > >  > 
 > >  > > Variable SLAAC [Can be activated via sysctl]: 
 > >  > > SLAAC with prefixes of arbitrary length in PIO (randomly 
 > >  > > generated hostID or stable privacy + privacy extensions). 
 > >  > > The main problem is that SLAAC RA or PD allocates a /64 by the Wireless 
 > >  > > carrier 4G, 5G to a mobile hotspot, however segmentation of the /64 via 
 > >  > > SLAAC is required so that downstream interfaces can be further subnetted. 
 > >  > > Example: uCPE device (4G + WI-FI enabled) receives /64 via Wireless, and 
 > >  > > assigns /72 to VNF-Firewall, /72 to WIFI, /72 to VNF-Router, /72 to 
 > >  > > Load-Balancer and /72 to wired connected devices. 
 > >  > > IETF document that defines problem statement: 
 > >  > > draft-mishra-v6ops-variable-slaac-problem-stmt 
 > >  > > IETF document that specifies variable slaac: 
 > >  > > draft-mishra-6man-variable-slaac 
 > >  > > 
 > >  > > Signed-off-by: Dmytro Shytyi <dmytro@xxxxxxxxxx> 
 > >  > > --- 
 > >  > > diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h 
 > >  > > index dda61d150a13..67ca3925463c 100644 
 > >  > > --- a/include/linux/ipv6.h 
 > >  > > +++ b/include/linux/ipv6.h 
 > >  > > @@ -75,6 +75,7 @@ struct ipv6_devconf { 
 > >  > >      __s32        disable_policy; 
 > >  > >      __s32           ndisc_tclass; 
 > >  > >      __s32        rpl_seg_enabled; 
 > >  > > +    __s32        variable_slaac; 
 > >  > > 
 > >  > >      struct ctl_table_header *sysctl_header; 
 > >  > >  }; 
 > >  > > diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h 
 > >  > > index 13e8751bf24a..f2af4f9fba2d 100644 
 > >  > > --- a/include/uapi/linux/ipv6.h 
 > >  > > +++ b/include/uapi/linux/ipv6.h 
 > >  > > @@ -189,7 +189,8 @@ enum { 
 > >  > >      DEVCONF_ACCEPT_RA_RT_INFO_MIN_PLEN, 
 > >  > >      DEVCONF_NDISC_TCLASS, 
 > >  > >      DEVCONF_RPL_SEG_ENABLED, 
 > >  > > -    DEVCONF_MAX 
 > >  > > +    DEVCONF_MAX, 
 > >  > > +    DEVCONF_VARIABLE_SLAAC 
 > >  > >  }; 
 > >  > > 
 > >  > > 
 > >  > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c 
 > >  > > index eff2cacd5209..07afe4ce984e 100644 
 > >  > > --- a/net/ipv6/addrconf.c 
 > >  > > +++ b/net/ipv6/addrconf.c 
 > >  > > @@ -236,6 +236,7 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = { 
 > >  > >      .addr_gen_mode        = IN6_ADDR_GEN_MODE_EUI64, 
 > >  > >      .disable_policy        = 0, 
 > >  > >      .rpl_seg_enabled    = 0, 
 > >  > > +    .variable_slaac        = 0, 
 > >  > >  }; 
 > >  > > 
 > >  > >  static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = { 
 > >  > > @@ -291,6 +292,7 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = { 
 > >  > >      .addr_gen_mode        = IN6_ADDR_GEN_MODE_EUI64, 
 > >  > >      .disable_policy        = 0, 
 > >  > >      .rpl_seg_enabled    = 0, 
 > >  > > +    .variable_slaac        = 0, 
 > >  > >  }; 
 > >  > > 
 > >  > >  /* Check if link is ready: is it up and is a valid qdisc available */ 
 > >  > > @@ -1340,9 +1342,15 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block) 
 > >  > >          goto out; 
 > >  > >      } 
 > >  > >      in6_ifa_hold(ifp); 
 > >  > > -    memcpy(addr.s6_addr, ifp->addr.s6_addr, 8); 
 > >  > > -    ipv6_gen_rnd_iid(&addr); 
 > >  > > 
 > >  > > +    if (ifp->prefix_len == 64) { 
 > >  > > +        memcpy(addr.s6_addr, ifp->addr.s6_addr, 8); 
 > >  > > +        ipv6_gen_rnd_iid(&addr); 
 > >  > > +    } else if (ifp->prefix_len > 0 && ifp->prefix_len <= 128 && 
 > >  > > +           idev->cnf.variable_slaac) { 
 > >  > > +        get_random_bytes(addr.s6_addr, 16); 
 > >  > > +        ipv6_addr_prefix_copy(&addr, &ifp->addr, ifp->prefix_len); 
 > >  > > +    } 
 > >  > >      age = (now - ifp->tstamp) / HZ; 
 > >  > > 
 > >  > >      regen_advance = idev->cnf.regen_max_retry * 
 > >  > > @@ -2569,6 +2577,37 @@ static bool is_addr_mode_generate_stable(struct inet6_dev *idev) 
 > >  > >             idev->cnf.addr_gen_mode == IN6_ADDR_GEN_MODE_RANDOM; 
 > >  > >  } 
 > >  > > 
 > >  > > +static struct inet6_ifaddr *ipv6_cmp_rcvd_prsnt_prfxs(struct inet6_ifaddr *ifp, 
 > >  > > +                              struct inet6_dev *in6_dev, 
 > >  > > +                              struct net *net, 
 > >  > > +                              const struct prefix_info *pinfo) 
 > >  > > +{ 
 > >  > > +    struct inet6_ifaddr *result_base = NULL; 
 > >  > > +    struct inet6_ifaddr *result = NULL; 
 > >  > > +    bool prfxs_equal; 
 > >  > > + 
 > >  > > +    result_base = result; 
 > >  > 
 > >  > This is NULL, are you sure you didn't mewan to init this to 'ifp' 
 > >  >  or similar instead? 
 > > 
 > > [Dmytro] I put the entire function to comment below the instructions. 
 > > [Dmytro]: 
 > > +static struct inet6_ifaddr *ipv6_cmp_rcvd_prsnt_prfxs(struct inet6_ifaddr *ifp, 
 > > +                         struct inet6_dev *in6_dev, 
 > > +                         struct net *net, 
 > > +                         const struct prefix_info *pinfo) 
 > > +{ 
 > > +    struct inet6_ifaddr *result_base = NULL; 
 > > +    struct inet6_ifaddr *result = NULL; 
 > > +    bool prfxs_equal; 
 > > + 
 > > +    result_base = result; 
 > > +    rcu_read_lock(); 
 > > +    list_for_each_entry_rcu(ifp, &in6_dev->addr_list, if_list) { 
 > > +        if (!net_eq(dev_net(ifp->idev->dev), net)) 
 > > +            continue; 
 > > +        prfxs_equal = 
 > > +            ipv6_prefix_equal(&pinfo->prefix, &ifp->addr, pinfo->prefix_len); 
 > > +        if (prfxs_equal && pinfo->prefix_len == ifp->prefix_len) { 
 > > +            result = ifp; 
 > > +            in6_ifa_hold(ifp); 
 > > +            break; 
 > > +        } 
 > > +    } 
 > > +    rcu_read_unlock(); 
 > > +    if (result_base != result) 
 > > +        ifp = result; 
 > > +    else 
 > > +        ifp = NULL; 
 > > + 
 > > +    return ifp; 
 > > +} 
 > > + 
 > > 
 > > [Dmytro]: 
 > > 1st initial stage is : 
 > > +    result_base = result; 
 > > 
 > > 2nd stage is (as you mention, 'result' will be assigned to 'ifp', in the process): 
 > > +            result = ifp; 
 > > 
 > > 3rd stage is to compare if  "result_base" and "result" are not equal (and take required action). 
 > >  if (result_base != result) 
 > > +        ifp = result; 
 > > +    else 
 > > +        ifp = NULL; 
 > > 
 > > Looks more/less ok for me. 
 >  
 > I think I see what you're trying to do here. Use result_base as a 
 > "marker" or the base value? 
 >  
 > But I'd say it makes the code harder to follow. It looks like this: 
 >  
 >     result_base = NULL; 
 >     result = NULL; 
 >  
 >     result_base = result 
 >     lock() 
 >     for ... 
 >         /* search logic */ 
 >     unlock() 
 >      
 >     if (result == result_base) 
 >         ifp = result 
 >     else 
 >         ifp = NULL 
 >     return NULL 
 >  
 > This would be a lot simpler, and functionally equivalent: 
 >  
 >     result = NULL 
 >  
 >     lock() 
 >     for ... 
 >         /* search logic */ 
 >     unlock() 
 >  
 >     return result 
 >  
 > Right? 
 > 
[Dmytro]: I see and I agree. Understood.