| 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(-)