Thanks for the review, I sent out a v4. On Fri, Sep 14, 2018 at 4:41 PM Justin Pettit <[email protected]> wrote:
> > > On Sep 14, 2018, at 1:20 PM, Yifeng Sun <[email protected]> wrote: > > > > @@ -1710,9 +1710,14 @@ static void ip6gre_destroy_tunnels(struct net > *net, struct list_head *head) > > static int __net_init ip6gre_init_net(struct net *net) > > { > > struct ip6gre_net *ign = net_generic(net, ip6gre_net_id); > > + const char *dev_name = "ovs-ip6gre0"; > > int err; > > > > - ign->fb_tunnel_dev = alloc_netdev(sizeof(struct ip6_tnl), > "ip6gre0", > > + if (strlen(dev_name) > IFNAMSIZ) > > + return -E2BIG; > > + > > + ign->fb_tunnel_dev = alloc_netdev(sizeof(struct ip6_tnl), > > + dev_name, > > NET_NAME_UNKNOWN, > > ip6gre_tunnel_setup); > > This string is allocated statically, so I don't think you need to do any > checks on its length. > > > if (!ign->fb_tunnel_dev) { > > diff --git a/datapath/linux/compat/ip6_tunnel.c > b/datapath/linux/compat/ip6_tunnel.c > > index 56fd8b4dd342..a7a1a586c920 100644 > > --- a/datapath/linux/compat/ip6_tunnel.c > > +++ b/datapath/linux/compat/ip6_tunnel.c > > @@ -355,7 +355,7 @@ static struct ip6_tnl *ip6_tnl_create(struct net > *net, struct __ip6_tnl_parm *p) > > if (p->name[0]) > > strlcpy(name, p->name, IFNAMSIZ); > > else > > - sprintf(name, "ip6tnl%%d"); > > + strlcpy(name, "ovs-ip6tnl%%d", IFNAMSIZ); > > I think you need to drop one of the "%", since it changed from a sprintf() > to a strlcpy(). > > The string length concern I'd raised in v1 was around the run-time > replacement of these "%d" arguments. I checked with Greg, and he said that > the number is not greater than three digits, so we should be fine with this > change, since we have room for up to a five digit number. > > > @@ -2087,13 +2087,17 @@ static int __net_init ip6_tnl_init_net(struct > net *net) > > { > > struct ip6_tnl_net *ip6n = net_generic(net, ip6_tnl_net_id); > > struct ip6_tnl *t = NULL; > > + const char *dev_name = "ovs-ip6tnl0"; > > int err; > > > > + if (strlen(dev_name) > IFNAMSIZ) > > + return -E2BIG; > > + > > ip6n->tnls[0] = ip6n->tnls_wc; > > ip6n->tnls[1] = ip6n->tnls_r_l; > > > > err = -ENOMEM; > > - ip6n->fb_tnl_dev = alloc_netdev(sizeof(struct ip6_tnl), "ip6tnl0", > > + ip6n->fb_tnl_dev = alloc_netdev(sizeof(struct ip6_tnl), dev_name, > > NET_NAME_UNKNOWN, > ip6_tnl_dev_setup); > > Again, I don't think you need run-time checks for this statically > allocated string. > > > static void __net_exit ipgre_tap_exit_net(struct net *net) > > diff --git a/datapath/linux/compat/ip_tunnel.c > b/datapath/linux/compat/ip_tunnel.c > > index d16e60fbfef0..0da8a06ecd26 100644 > > --- a/datapath/linux/compat/ip_tunnel.c > > +++ b/datapath/linux/compat/ip_tunnel.c > > @@ -130,11 +130,12 @@ static struct net_device > *__ip_tunnel_create(struct net *net, > > if (parms->name[0]) > > strlcpy(name, parms->name, IFNAMSIZ); > > else { > > - if (strlen(ops->kind) > (IFNAMSIZ - 3)) { > > + if (strlen(ops->kind) > (IFNAMSIZ - 7)) { > > I think is correct that we need to increase this by four, since we added > "ovs-". However, if the largest replacement for "%d" is three digits, > should that check upstream be for "IFNAMESIZ - 4" to account for the > nul-terminator? This is a question that applies to the Linux kernel > upstream, too. > > > err = -E2BIG; > > goto failed; > > } > > - strlcpy(name, ops->kind, IFNAMSIZ); > > + strlcpy(name, "ovs-", IFNAMSIZ); > > + strlcat(name, ops->kind, IFNAMSIZ); > > strncat(name, "%d", 2); > > This isn't new to your changes, but we've already done string length > sanity checks earlier, so these aren't adding much, and we don't even both > with the last one, which would be the most likely to overflow. Regardless, > as we discussed offline, it might be just cleaner to use sprintf() or > snprintf(). > > --Justin > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
