* Re: [PATCH ipsec-next] xfrm: check for xdo_dev_state_free
2017-12-11 20:57 [PATCH ipsec-next] xfrm: check for xdo_dev_state_free Shannon Nelson
@ 2017-12-14 6:20 ` Steffen Klassert
2017-12-14 16:28 ` Shannon Nelson
2017-12-14 7:45 ` kbuild test robot
2017-12-14 8:51 ` kbuild test robot
2 siblings, 1 reply; 5+ messages in thread
From: Steffen Klassert @ 2017-12-14 6:20 UTC (permalink / raw)
To: Shannon Nelson; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH ipsec-next] xfrm: check for xdo_dev_state_free
2017-12-11 20:57 [PATCH ipsec-next] xfrm: check for xdo_dev_state_free Shannon Nelson
2017-12-14 6:20 ` Steffen Klassert
@ 2017-12-14 7:45 ` kbuild test robot
2017-12-14 8:51 ` kbuild test robot
2 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2017-12-14 7:45 UTC (permalink / raw)
To: Shannon Nelson; +Cc: kbuild-all, steffen.klassert, netdev
[-- Attachment #1: Type: text/plain, Size: 5842 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30405 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread