Message ID | 1513025842-12064-1-git-send-email-shannon.nelson@oracle.com |
---|---|
State | Awaiting Upstream, archived |
Delegated to: | David Miller |
Headers | show |
Series | [ipsec-next] xfrm: check for xdo_dev_state_free | expand |
On Mon, Dec 11, 2017 at 12:57:22PM -0800, Shannon Nelson wrote: > The current XFRM code assumes that we've implemented the > xdo_dev_state_free() callback, even if it is meaningless to the driver. > This patch adds a check for it before calling, as done in other APIs, > and is done for the xdo_state_offload_ok() callback. > > Also, we add a check for the required add and delete functions up front > at registration time to be sure both are defined, and complain if not. > > Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com> > --- > include/net/xfrm.h | 3 ++- > net/xfrm/xfrm_device.c | 18 ++++++++++++++---- > 2 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h > index e015e16..dfabd04 100644 > --- a/include/net/xfrm.h > +++ b/include/net/xfrm.h > @@ -1891,7 +1891,8 @@ static inline void xfrm_dev_state_free(struct xfrm_state *x) > struct net_device *dev = xso->dev; > > if (dev && dev->xfrmdev_ops) { > - dev->xfrmdev_ops->xdo_dev_state_free(x); > + if (dev->xfrmdev_ops->xdo_dev_state_free) > + dev->xfrmdev_ops->xdo_dev_state_free(x); > xso->dev = NULL; > dev_put(dev); > } > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c > index 30e5746..0df1cc2 100644 > --- a/net/xfrm/xfrm_device.c > +++ b/net/xfrm/xfrm_device.c > @@ -144,11 +144,21 @@ EXPORT_SYMBOL_GPL(xfrm_dev_offload_ok); > > static int xfrm_dev_register(struct net_device *dev) > { > - if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops) > - return NOTIFY_BAD; > - if ((dev->features & NETIF_F_HW_ESP_TX_CSUM) && > - !(dev->features & NETIF_F_HW_ESP)) > + if (!(dev->features & NETIF_F_HW_ESP)) { > + if (dev->features & NETIF_F_HW_ESP_TX_CSUM) { > + netdev_err(dev, "NETIF_F_HW_ESP_TX_CSUM without NETIF_F_HW_ESP\n"); > + return NOTIFY_BAD; > + } else { > + return NOTIFY_DONE; > + } > + } > + > + if (!(dev->xfrmdev_ops && > + dev->xfrmdev_ops->xdo_dev_state_add && > + dev->xfrmdev_ops->xdo_dev_state_delete)) { > + netdev_err(dev, "add or delete function missing from xfrmdev_ops\n"); Please remove these error printings, this is not relevant for normal users.
Hi Shannon, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on ipsec-next/master] url: https://github.com/0day-ci/linux/commits/Shannon-Nelson/xfrm-check-for-xdo_dev_state_free/20171214-150202 base: https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec-next.git master config: x86_64-randconfig-x002-201750 (attached as .config) compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): In file included from include/linux/kernel.h:10:0, from include/linux/list.h:9, from include/linux/module.h:9, from net/xfrm/xfrm_device.c:16: net/xfrm/xfrm_device.c: In function 'xfrm_dev_register': net/xfrm/xfrm_device.c:157:24: error: dereferencing pointer to incomplete type 'const struct xfrmdev_ops' dev->xfrmdev_ops->xdo_dev_state_add && ^ include/linux/compiler.h:58:30: note: in definition of macro '__trace_if' if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ ^~~~ >> net/xfrm/xfrm_device.c:156:2: note: in expansion of macro 'if' if (!(dev->xfrmdev_ops && ^~ vim +/if +156 net/xfrm/xfrm_device.c > 16 #include <linux/module.h> 17 #include <linux/netdevice.h> 18 #include <linux/skbuff.h> 19 #include <linux/slab.h> 20 #include <linux/spinlock.h> 21 #include <net/dst.h> 22 #include <net/xfrm.h> 23 #include <linux/notifier.h> 24 25 #ifdef CONFIG_XFRM_OFFLOAD 26 int validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t features) 27 { 28 int err; 29 struct xfrm_state *x; 30 struct xfrm_offload *xo = xfrm_offload(skb); 31 32 if (skb_is_gso(skb)) 33 return 0; 34 35 if (xo) { 36 x = skb->sp->xvec[skb->sp->len - 1]; 37 if (xo->flags & XFRM_GRO || x->xso.flags & XFRM_OFFLOAD_INBOUND) 38 return 0; 39 40 x->outer_mode->xmit(x, skb); 41 42 err = x->type_offload->xmit(x, skb, features); 43 if (err) { 44 XFRM_INC_STATS(xs_net(x), LINUX_MIB_XFRMOUTSTATEPROTOERROR); 45 return err; 46 } 47 48 skb_push(skb, skb->data - skb_mac_header(skb)); 49 } 50 51 return 0; 52 } 53 EXPORT_SYMBOL_GPL(validate_xmit_xfrm); 54 55 int xfrm_dev_state_add(struct net *net, struct xfrm_state *x, 56 struct xfrm_user_offload *xuo) 57 { 58 int err; 59 struct dst_entry *dst; 60 struct net_device *dev; 61 struct xfrm_state_offload *xso = &x->xso; 62 xfrm_address_t *saddr; 63 xfrm_address_t *daddr; 64 65 if (!x->type_offload) 66 return -EINVAL; 67 68 /* We don't yet support UDP encapsulation, TFC padding and ESN. */ 69 if (x->encap || x->tfcpad || (x->props.flags & XFRM_STATE_ESN)) 70 return -EINVAL; 71 72 dev = dev_get_by_index(net, xuo->ifindex); 73 if (!dev) { 74 if (!(xuo->flags & XFRM_OFFLOAD_INBOUND)) { 75 saddr = &x->props.saddr; 76 daddr = &x->id.daddr; 77 } else { 78 saddr = &x->id.daddr; 79 daddr = &x->props.saddr; 80 } 81 82 dst = __xfrm_dst_lookup(net, 0, 0, saddr, daddr, 83 x->props.family, x->props.output_mark); 84 if (IS_ERR(dst)) 85 return 0; 86 87 dev = dst->dev; 88 89 dev_hold(dev); 90 dst_release(dst); 91 } 92 93 if (!dev->xfrmdev_ops || !dev->xfrmdev_ops->xdo_dev_state_add) { 94 xso->dev = NULL; 95 dev_put(dev); 96 return 0; 97 } 98 99 xso->dev = dev; 100 xso->num_exthdrs = 1; 101 xso->flags = xuo->flags; 102 103 err = dev->xfrmdev_ops->xdo_dev_state_add(x); 104 if (err) { 105 dev_put(dev); 106 return err; 107 } 108 109 return 0; 110 } 111 EXPORT_SYMBOL_GPL(xfrm_dev_state_add); 112 113 bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x) 114 { 115 int mtu; 116 struct dst_entry *dst = skb_dst(skb); 117 struct xfrm_dst *xdst = (struct xfrm_dst *)dst; 118 struct net_device *dev = x->xso.dev; 119 120 if (!x->type_offload || x->encap) 121 return false; 122 123 if ((x->xso.offload_handle && (dev == dst->path->dev)) && 124 !dst->child->xfrm && x->type->get_mtu) { 125 mtu = x->type->get_mtu(x, xdst->child_mtu_cached); 126 127 if (skb->len <= mtu) 128 goto ok; 129 130 if (skb_is_gso(skb) && skb_gso_validate_mtu(skb, mtu)) 131 goto ok; 132 } 133 134 return false; 135 136 ok: 137 if (dev && dev->xfrmdev_ops && dev->xfrmdev_ops->xdo_dev_offload_ok) 138 return x->xso.dev->xfrmdev_ops->xdo_dev_offload_ok(skb, x); 139 140 return true; 141 } 142 EXPORT_SYMBOL_GPL(xfrm_dev_offload_ok); 143 #endif 144 145 static int xfrm_dev_register(struct net_device *dev) 146 { 147 if (!(dev->features & NETIF_F_HW_ESP)) { 148 if (dev->features & NETIF_F_HW_ESP_TX_CSUM) { 149 netdev_err(dev, "NETIF_F_HW_ESP_TX_CSUM without NETIF_F_HW_ESP\n"); 150 return NOTIFY_BAD; 151 } else { 152 return NOTIFY_DONE; 153 } 154 } 155 > 156 if (!(dev->xfrmdev_ops && 157 dev->xfrmdev_ops->xdo_dev_state_add && 158 dev->xfrmdev_ops->xdo_dev_state_delete)) { 159 netdev_err(dev, "add or delete function missing from xfrmdev_ops\n"); 160 return NOTIFY_BAD; 161 } 162 163 return NOTIFY_DONE; 164 } 165 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Shannon, Thank you for the patch! Yet something to improve: [auto build test ERROR on ipsec-next/master] url: https://github.com/0day-ci/linux/commits/Shannon-Nelson/xfrm-check-for-xdo_dev_state_free/20171214-150202 base: https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec-next.git master config: score-spct6600_defconfig (attached as .config) compiler: score-elf-gcc (GCC) 4.9.4 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=score All errors (new ones prefixed by >>): net/xfrm/xfrm_device.c: In function 'xfrm_dev_register': >> net/xfrm/xfrm_device.c:157:24: error: dereferencing pointer to incomplete type dev->xfrmdev_ops->xdo_dev_state_add && ^ net/xfrm/xfrm_device.c:158:24: error: dereferencing pointer to incomplete type dev->xfrmdev_ops->xdo_dev_state_delete)) { ^ vim +157 net/xfrm/xfrm_device.c 144 145 static int xfrm_dev_register(struct net_device *dev) 146 { 147 if (!(dev->features & NETIF_F_HW_ESP)) { 148 if (dev->features & NETIF_F_HW_ESP_TX_CSUM) { 149 netdev_err(dev, "NETIF_F_HW_ESP_TX_CSUM without NETIF_F_HW_ESP\n"); 150 return NOTIFY_BAD; 151 } else { 152 return NOTIFY_DONE; 153 } 154 } 155 156 if (!(dev->xfrmdev_ops && > 157 dev->xfrmdev_ops->xdo_dev_state_add && 158 dev->xfrmdev_ops->xdo_dev_state_delete)) { 159 netdev_err(dev, "add or delete function missing from xfrmdev_ops\n"); 160 return NOTIFY_BAD; 161 } 162 163 return NOTIFY_DONE; 164 } 165 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 12/13/2017 10:20 PM, Steffen Klassert wrote: > On Mon, Dec 11, 2017 at 12:57:22PM -0800, Shannon Nelson wrote: >> The current XFRM code assumes that we've implemented the >> xdo_dev_state_free() callback, even if it is meaningless to the driver. <snip> >> + if (dev->features & NETIF_F_HW_ESP_TX_CSUM) { >> + netdev_err(dev, "NETIF_F_HW_ESP_TX_CSUM without NETIF_F_HW_ESP\n"); >> + return NOTIFY_BAD; >> + } else { >> + return NOTIFY_DONE; >> + } >> + } >> + >> + if (!(dev->xfrmdev_ops && >> + dev->xfrmdev_ops->xdo_dev_state_add && >> + dev->xfrmdev_ops->xdo_dev_state_delete)) { >> + netdev_err(dev, "add or delete function missing from xfrmdev_ops\n"); > > Please remove these error printings, this is not relevant for normal > users. > Okay. After I posted this I realized this really should be two patches, so I'll split this up as well before resending. sln
diff --git a/include/net/xfrm.h b/include/net/xfrm.h index e015e16..dfabd04 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -1891,7 +1891,8 @@ static inline void xfrm_dev_state_free(struct xfrm_state *x) struct net_device *dev = xso->dev; if (dev && dev->xfrmdev_ops) { - dev->xfrmdev_ops->xdo_dev_state_free(x); + if (dev->xfrmdev_ops->xdo_dev_state_free) + dev->xfrmdev_ops->xdo_dev_state_free(x); xso->dev = NULL; dev_put(dev); } diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c index 30e5746..0df1cc2 100644 --- a/net/xfrm/xfrm_device.c +++ b/net/xfrm/xfrm_device.c @@ -144,11 +144,21 @@ EXPORT_SYMBOL_GPL(xfrm_dev_offload_ok); static int xfrm_dev_register(struct net_device *dev) { - if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops) - return NOTIFY_BAD; - if ((dev->features & NETIF_F_HW_ESP_TX_CSUM) && - !(dev->features & NETIF_F_HW_ESP)) + if (!(dev->features & NETIF_F_HW_ESP)) { + if (dev->features & NETIF_F_HW_ESP_TX_CSUM) { + netdev_err(dev, "NETIF_F_HW_ESP_TX_CSUM without NETIF_F_HW_ESP\n"); + return NOTIFY_BAD; + } else { + return NOTIFY_DONE; + } + } + + if (!(dev->xfrmdev_ops && + dev->xfrmdev_ops->xdo_dev_state_add && + dev->xfrmdev_ops->xdo_dev_state_delete)) { + netdev_err(dev, "add or delete function missing from xfrmdev_ops\n"); return NOTIFY_BAD; + } return NOTIFY_DONE; }
The current XFRM code assumes that we've implemented the xdo_dev_state_free() callback, even if it is meaningless to the driver. This patch adds a check for it before calling, as done in other APIs, and is done for the xdo_state_offload_ok() callback. Also, we add a check for the required add and delete functions up front at registration time to be sure both are defined, and complain if not. Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com> --- include/net/xfrm.h | 3 ++- net/xfrm/xfrm_device.c | 18 ++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-)