* pull request (net): ipsec 2021-08-04
@ 2021-08-04 7:03 Steffen Klassert
2021-08-04 7:03 ` [PATCH 1/6] net: xfrm: fix memory leak in xfrm_user_rcv_msg Steffen Klassert
` (6 more replies)
0 siblings, 7 replies; 9+ messages in thread
From: Steffen Klassert @ 2021-08-04 7:03 UTC (permalink / raw)
To: David Miller, Jakub Kicinski; +Cc: Herbert Xu, Steffen Klassert, netdev
1) Fix a sysbot reported memory leak in xfrm_user_rcv_msg.
From Pavel Skripkin.
2) Revert "xfrm: policy: Read seqcount outside of rcu-read side
in xfrm_policy_lookup_bytype". This commit tried to fix a
lockin bug, but only cured some of the symptoms. A proper
fix is applied on top of this revert.
3) Fix a locking bug on xfrm state hash resize. A recent change
on sequence counters accidentally repaced a spinlock by a mutex.
Fix from Frederic Weisbecker.
4) Fix possible user-memory-access in xfrm_user_rcv_msg_compat().
From Dmitry Safonov.
5) Add initialiation sefltest fot xfrm_spdattr_type_t.
From Dmitry Safonov.
Please pull or let me know if there are problems.
Thanks!
The following changes since commit a118ff661889ecee3ca90f8125bad8fb5bbc07d5:
selftests: net: devlink_port_split: check devlink returned an element before dereferencing it (2021-06-28 16:14:38 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git master
for you to fetch changes up to 480e93e12aa04d857f7cc2e6fcec181c0d690404:
net: xfrm: Fix end of loop tests for list_for_each_entry (2021-07-26 12:26:28 +0200)
----------------------------------------------------------------
Dmitry Safonov (2):
net/xfrm/compat: Copy xfrm_spdattr_type_t atributes
selftests/net/ipsec: Add test for xfrm_spdattr_type_t
Frederic Weisbecker (1):
xfrm: Fix RCU vs hash_resize_mutex lock inversion
Harshvardhan Jha (1):
net: xfrm: Fix end of loop tests for list_for_each_entry
Pavel Skripkin (1):
net: xfrm: fix memory leak in xfrm_user_rcv_msg
Steffen Klassert (2):
Revert "xfrm: policy: Read seqcount outside of rcu-read side in xfrm_policy_lookup_bytype"
Merge branch 'xfrm/compat: Fix xfrm_spdattr_type_t copying'
include/net/netns/xfrm.h | 1 +
net/xfrm/xfrm_compat.c | 49 +++++++++--
net/xfrm/xfrm_ipcomp.c | 2 +-
net/xfrm/xfrm_policy.c | 32 +++----
net/xfrm/xfrm_user.c | 10 +++
tools/testing/selftests/net/ipsec.c | 165 +++++++++++++++++++++++++++++++++++-
6 files changed, 231 insertions(+), 28 deletions(-)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/6] net: xfrm: fix memory leak in xfrm_user_rcv_msg
2021-08-04 7:03 pull request (net): ipsec 2021-08-04 Steffen Klassert
@ 2021-08-04 7:03 ` Steffen Klassert
2021-08-04 10:10 ` patchwork-bot+netdevbpf
2021-08-04 7:03 ` [PATCH 2/6] Revert "xfrm: policy: Read seqcount outside of rcu-read side in xfrm_policy_lookup_bytype" Steffen Klassert
` (5 subsequent siblings)
6 siblings, 1 reply; 9+ messages in thread
From: Steffen Klassert @ 2021-08-04 7:03 UTC (permalink / raw)
To: David Miller, Jakub Kicinski; +Cc: Herbert Xu, Steffen Klassert, netdev
From: Pavel Skripkin <paskripkin@gmail.com>
Syzbot reported memory leak in xfrm_user_rcv_msg(). The
problem was is non-freed skb's frag_list.
In skb_release_all() skb_release_data() will be called only
in case of skb->head != NULL, but netlink_skb_destructor()
sets head to NULL. So, allocated frag_list skb should be
freed manualy, since consume_skb() won't take care of it
Fixes: 5106f4a8acff ("xfrm/compat: Add 32=>64-bit messages translator")
Reported-and-tested-by: syzbot+fb347cf82c73a90efcca@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/xfrm/xfrm_user.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index b47d613409b7..7aff641c717d 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2811,6 +2811,16 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
err = link->doit(skb, nlh, attrs);
+ /* We need to free skb allocated in xfrm_alloc_compat() before
+ * returning from this function, because consume_skb() won't take
+ * care of frag_list since netlink destructor sets
+ * sbk->head to NULL. (see netlink_skb_destructor())
+ */
+ if (skb_has_frag_list(skb)) {
+ kfree_skb(skb_shinfo(skb)->frag_list);
+ skb_shinfo(skb)->frag_list = NULL;
+ }
+
err:
kvfree(nlh64);
return err;
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/6] Revert "xfrm: policy: Read seqcount outside of rcu-read side in xfrm_policy_lookup_bytype"
2021-08-04 7:03 pull request (net): ipsec 2021-08-04 Steffen Klassert
2021-08-04 7:03 ` [PATCH 1/6] net: xfrm: fix memory leak in xfrm_user_rcv_msg Steffen Klassert
@ 2021-08-04 7:03 ` Steffen Klassert
2021-08-04 7:03 ` [PATCH 3/6] xfrm: Fix RCU vs hash_resize_mutex lock inversion Steffen Klassert
` (4 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2021-08-04 7:03 UTC (permalink / raw)
To: David Miller, Jakub Kicinski; +Cc: Herbert Xu, Steffen Klassert, netdev
This reverts commit d7b0408934c749f546b01f2b33d07421a49b6f3e.
This commit tried to fix a locking bug introduced by commit 77cc278f7b20
("xfrm: policy: Use sequence counters with associated lock"). As it
turned out, this patch did not really fix the bug. A proper fix
for this bug is applied on top of this revert.
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/xfrm/xfrm_policy.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index e9d0df2a2ab1..ce500f847b99 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2092,15 +2092,12 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
if (unlikely(!daddr || !saddr))
return NULL;
- retry:
- sequence = read_seqcount_begin(&xfrm_policy_hash_generation);
rcu_read_lock();
-
- chain = policy_hash_direct(net, daddr, saddr, family, dir);
- if (read_seqcount_retry(&xfrm_policy_hash_generation, sequence)) {
- rcu_read_unlock();
- goto retry;
- }
+ retry:
+ do {
+ sequence = read_seqcount_begin(&xfrm_policy_hash_generation);
+ chain = policy_hash_direct(net, daddr, saddr, family, dir);
+ } while (read_seqcount_retry(&xfrm_policy_hash_generation, sequence));
ret = NULL;
hlist_for_each_entry_rcu(pol, chain, bydst) {
@@ -2131,15 +2128,11 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
}
skip_inexact:
- if (read_seqcount_retry(&xfrm_policy_hash_generation, sequence)) {
- rcu_read_unlock();
+ if (read_seqcount_retry(&xfrm_policy_hash_generation, sequence))
goto retry;
- }
- if (ret && !xfrm_pol_hold_rcu(ret)) {
- rcu_read_unlock();
+ if (ret && !xfrm_pol_hold_rcu(ret))
goto retry;
- }
fail:
rcu_read_unlock();
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/6] xfrm: Fix RCU vs hash_resize_mutex lock inversion
2021-08-04 7:03 pull request (net): ipsec 2021-08-04 Steffen Klassert
2021-08-04 7:03 ` [PATCH 1/6] net: xfrm: fix memory leak in xfrm_user_rcv_msg Steffen Klassert
2021-08-04 7:03 ` [PATCH 2/6] Revert "xfrm: policy: Read seqcount outside of rcu-read side in xfrm_policy_lookup_bytype" Steffen Klassert
@ 2021-08-04 7:03 ` Steffen Klassert
2021-08-04 7:03 ` [PATCH 4/6] net/xfrm/compat: Copy xfrm_spdattr_type_t atributes Steffen Klassert
` (3 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2021-08-04 7:03 UTC (permalink / raw)
To: David Miller, Jakub Kicinski; +Cc: Herbert Xu, Steffen Klassert, netdev
From: Frederic Weisbecker <frederic@kernel.org>
xfrm_bydst_resize() calls synchronize_rcu() while holding
hash_resize_mutex. But then on PREEMPT_RT configurations,
xfrm_policy_lookup_bytype() may acquire that mutex while running in an
RCU read side critical section. This results in a deadlock.
In fact the scope of hash_resize_mutex is way beyond the purpose of
xfrm_policy_lookup_bytype() to just fetch a coherent and stable policy
for a given destination/direction, along with other details.
The lower level net->xfrm.xfrm_policy_lock, which among other things
protects per destination/direction references to policy entries, is
enough to serialize and benefit from priority inheritance against the
write side. As a bonus, it makes it officially a per network namespace
synchronization business where a policy table resize on namespace A
shouldn't block a policy lookup on namespace B.
Fixes: 77cc278f7b20 (xfrm: policy: Use sequence counters with associated lock)
Cc: stable@vger.kernel.org
Cc: Ahmed S. Darwish <a.darwish@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Varad Gautam <varad.gautam@suse.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
include/net/netns/xfrm.h | 1 +
net/xfrm/xfrm_policy.c | 17 ++++++++---------
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
index e816b6a3ef2b..9b376b87bd54 100644
--- a/include/net/netns/xfrm.h
+++ b/include/net/netns/xfrm.h
@@ -74,6 +74,7 @@ struct netns_xfrm {
#endif
spinlock_t xfrm_state_lock;
seqcount_spinlock_t xfrm_state_hash_generation;
+ seqcount_spinlock_t xfrm_policy_hash_generation;
spinlock_t xfrm_policy_lock;
struct mutex xfrm_cfg_mutex;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index ce500f847b99..46a6d15b66d6 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -155,7 +155,6 @@ static struct xfrm_policy_afinfo const __rcu *xfrm_policy_afinfo[AF_INET6 + 1]
__read_mostly;
static struct kmem_cache *xfrm_dst_cache __ro_after_init;
-static __read_mostly seqcount_mutex_t xfrm_policy_hash_generation;
static struct rhashtable xfrm_policy_inexact_table;
static const struct rhashtable_params xfrm_pol_inexact_params;
@@ -585,7 +584,7 @@ static void xfrm_bydst_resize(struct net *net, int dir)
return;
spin_lock_bh(&net->xfrm.xfrm_policy_lock);
- write_seqcount_begin(&xfrm_policy_hash_generation);
+ write_seqcount_begin(&net->xfrm.xfrm_policy_hash_generation);
odst = rcu_dereference_protected(net->xfrm.policy_bydst[dir].table,
lockdep_is_held(&net->xfrm.xfrm_policy_lock));
@@ -596,7 +595,7 @@ static void xfrm_bydst_resize(struct net *net, int dir)
rcu_assign_pointer(net->xfrm.policy_bydst[dir].table, ndst);
net->xfrm.policy_bydst[dir].hmask = nhashmask;
- write_seqcount_end(&xfrm_policy_hash_generation);
+ write_seqcount_end(&net->xfrm.xfrm_policy_hash_generation);
spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
synchronize_rcu();
@@ -1245,7 +1244,7 @@ static void xfrm_hash_rebuild(struct work_struct *work)
} while (read_seqretry(&net->xfrm.policy_hthresh.lock, seq));
spin_lock_bh(&net->xfrm.xfrm_policy_lock);
- write_seqcount_begin(&xfrm_policy_hash_generation);
+ write_seqcount_begin(&net->xfrm.xfrm_policy_hash_generation);
/* make sure that we can insert the indirect policies again before
* we start with destructive action.
@@ -1354,7 +1353,7 @@ static void xfrm_hash_rebuild(struct work_struct *work)
out_unlock:
__xfrm_policy_inexact_flush(net);
- write_seqcount_end(&xfrm_policy_hash_generation);
+ write_seqcount_end(&net->xfrm.xfrm_policy_hash_generation);
spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
mutex_unlock(&hash_resize_mutex);
@@ -2095,9 +2094,9 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
rcu_read_lock();
retry:
do {
- sequence = read_seqcount_begin(&xfrm_policy_hash_generation);
+ sequence = read_seqcount_begin(&net->xfrm.xfrm_policy_hash_generation);
chain = policy_hash_direct(net, daddr, saddr, family, dir);
- } while (read_seqcount_retry(&xfrm_policy_hash_generation, sequence));
+ } while (read_seqcount_retry(&net->xfrm.xfrm_policy_hash_generation, sequence));
ret = NULL;
hlist_for_each_entry_rcu(pol, chain, bydst) {
@@ -2128,7 +2127,7 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
}
skip_inexact:
- if (read_seqcount_retry(&xfrm_policy_hash_generation, sequence))
+ if (read_seqcount_retry(&net->xfrm.xfrm_policy_hash_generation, sequence))
goto retry;
if (ret && !xfrm_pol_hold_rcu(ret))
@@ -4084,6 +4083,7 @@ static int __net_init xfrm_net_init(struct net *net)
/* Initialize the per-net locks here */
spin_lock_init(&net->xfrm.xfrm_state_lock);
spin_lock_init(&net->xfrm.xfrm_policy_lock);
+ seqcount_spinlock_init(&net->xfrm.xfrm_policy_hash_generation, &net->xfrm.xfrm_policy_lock);
mutex_init(&net->xfrm.xfrm_cfg_mutex);
rv = xfrm_statistics_init(net);
@@ -4128,7 +4128,6 @@ void __init xfrm_init(void)
{
register_pernet_subsys(&xfrm_net_ops);
xfrm_dev_init();
- seqcount_mutex_init(&xfrm_policy_hash_generation, &hash_resize_mutex);
xfrm_input_init();
#ifdef CONFIG_XFRM_ESPINTCP
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/6] net/xfrm/compat: Copy xfrm_spdattr_type_t atributes
2021-08-04 7:03 pull request (net): ipsec 2021-08-04 Steffen Klassert
` (2 preceding siblings ...)
2021-08-04 7:03 ` [PATCH 3/6] xfrm: Fix RCU vs hash_resize_mutex lock inversion Steffen Klassert
@ 2021-08-04 7:03 ` Steffen Klassert
2021-08-04 7:03 ` [PATCH 5/6] selftests/net/ipsec: Add test for xfrm_spdattr_type_t Steffen Klassert
` (2 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2021-08-04 7:03 UTC (permalink / raw)
To: David Miller, Jakub Kicinski; +Cc: Herbert Xu, Steffen Klassert, netdev
From: Dmitry Safonov <dima@arista.com>
The attribute-translator has to take in mind maxtype, that is
xfrm_link::nla_max. When it is set, attributes are not of xfrm_attr_type_t.
Currently, they can be only XFRMA_SPD_MAX (message XFRM_MSG_NEWSPDINFO),
their UABI is the same for 64/32-bit, so just copy them.
Thanks to YueHaibing for reporting this:
In xfrm_user_rcv_msg_compat() if maxtype is not zero and less than
XFRMA_MAX, nlmsg_parse_deprecated() do not initialize attrs array fully.
xfrm_xlate32() will access uninit 'attrs[i]' while iterating all attrs
array.
KASAN: probably user-memory-access in range [0x0000000041b58ab0-0x0000000041b58ab7]
CPU: 0 PID: 15799 Comm: syz-executor.2 Tainted: G W 5.14.0-rc1-syzkaller #0
RIP: 0010:nla_type include/net/netlink.h:1130 [inline]
RIP: 0010:xfrm_xlate32_attr net/xfrm/xfrm_compat.c:410 [inline]
RIP: 0010:xfrm_xlate32 net/xfrm/xfrm_compat.c:532 [inline]
RIP: 0010:xfrm_user_rcv_msg_compat+0x5e5/0x1070 net/xfrm/xfrm_compat.c:577
[...]
Call Trace:
xfrm_user_rcv_msg+0x556/0x8b0 net/xfrm/xfrm_user.c:2774
netlink_rcv_skb+0x153/0x420 net/netlink/af_netlink.c:2504
xfrm_netlink_rcv+0x6b/0x90 net/xfrm/xfrm_user.c:2824
netlink_unicast_kernel net/netlink/af_netlink.c:1314 [inline]
netlink_unicast+0x533/0x7d0 net/netlink/af_netlink.c:1340
netlink_sendmsg+0x86d/0xdb0 net/netlink/af_netlink.c:1929
sock_sendmsg_nosec net/socket.c:702 [inline]
Fixes: 5106f4a8acff ("xfrm/compat: Add 32=>64-bit messages translator")
Cc: <stable@kernel.org>
Reported-by: YueHaibing <yuehaibing@huawei.com>
Signed-off-by: Dmitry Safonov <dima@arista.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/xfrm/xfrm_compat.c | 49 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 44 insertions(+), 5 deletions(-)
diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
index a20aec9d7393..2bf269390163 100644
--- a/net/xfrm/xfrm_compat.c
+++ b/net/xfrm/xfrm_compat.c
@@ -298,8 +298,16 @@ static int xfrm_xlate64(struct sk_buff *dst, const struct nlmsghdr *nlh_src)
len = nlmsg_attrlen(nlh_src, xfrm_msg_min[type]);
nla_for_each_attr(nla, attrs, len, remaining) {
- int err = xfrm_xlate64_attr(dst, nla);
+ int err;
+ switch (type) {
+ case XFRM_MSG_NEWSPDINFO:
+ err = xfrm_nla_cpy(dst, nla, nla_len(nla));
+ break;
+ default:
+ err = xfrm_xlate64_attr(dst, nla);
+ break;
+ }
if (err)
return err;
}
@@ -341,7 +349,8 @@ static int xfrm_alloc_compat(struct sk_buff *skb, const struct nlmsghdr *nlh_src
/* Calculates len of translated 64-bit message. */
static size_t xfrm_user_rcv_calculate_len64(const struct nlmsghdr *src,
- struct nlattr *attrs[XFRMA_MAX+1])
+ struct nlattr *attrs[XFRMA_MAX + 1],
+ int maxtype)
{
size_t len = nlmsg_len(src);
@@ -358,10 +367,20 @@ static size_t xfrm_user_rcv_calculate_len64(const struct nlmsghdr *src,
case XFRM_MSG_POLEXPIRE:
len += 8;
break;
+ case XFRM_MSG_NEWSPDINFO:
+ /* attirbutes are xfrm_spdattr_type_t, not xfrm_attr_type_t */
+ return len;
default:
break;
}
+ /* Unexpected for anything, but XFRM_MSG_NEWSPDINFO, please
+ * correct both 64=>32-bit and 32=>64-bit translators to copy
+ * new attributes.
+ */
+ if (WARN_ON_ONCE(maxtype))
+ return len;
+
if (attrs[XFRMA_SA])
len += 4;
if (attrs[XFRMA_POLICY])
@@ -440,7 +459,8 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla,
static int xfrm_xlate32(struct nlmsghdr *dst, const struct nlmsghdr *src,
struct nlattr *attrs[XFRMA_MAX+1],
- size_t size, u8 type, struct netlink_ext_ack *extack)
+ size_t size, u8 type, int maxtype,
+ struct netlink_ext_ack *extack)
{
size_t pos;
int i;
@@ -520,6 +540,25 @@ static int xfrm_xlate32(struct nlmsghdr *dst, const struct nlmsghdr *src,
}
pos = dst->nlmsg_len;
+ if (maxtype) {
+ /* attirbutes are xfrm_spdattr_type_t, not xfrm_attr_type_t */
+ WARN_ON_ONCE(src->nlmsg_type != XFRM_MSG_NEWSPDINFO);
+
+ for (i = 1; i <= maxtype; i++) {
+ int err;
+
+ if (!attrs[i])
+ continue;
+
+ /* just copy - no need for translation */
+ err = xfrm_attr_cpy32(dst, &pos, attrs[i], size,
+ nla_len(attrs[i]), nla_len(attrs[i]));
+ if (err)
+ return err;
+ }
+ return 0;
+ }
+
for (i = 1; i < XFRMA_MAX + 1; i++) {
int err;
@@ -564,7 +603,7 @@ static struct nlmsghdr *xfrm_user_rcv_msg_compat(const struct nlmsghdr *h32,
if (err < 0)
return ERR_PTR(err);
- len = xfrm_user_rcv_calculate_len64(h32, attrs);
+ len = xfrm_user_rcv_calculate_len64(h32, attrs, maxtype);
/* The message doesn't need translation */
if (len == nlmsg_len(h32))
return NULL;
@@ -574,7 +613,7 @@ static struct nlmsghdr *xfrm_user_rcv_msg_compat(const struct nlmsghdr *h32,
if (!h64)
return ERR_PTR(-ENOMEM);
- err = xfrm_xlate32(h64, h32, attrs, len, type, extack);
+ err = xfrm_xlate32(h64, h32, attrs, len, type, maxtype, extack);
if (err < 0) {
kvfree(h64);
return ERR_PTR(err);
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 5/6] selftests/net/ipsec: Add test for xfrm_spdattr_type_t
2021-08-04 7:03 pull request (net): ipsec 2021-08-04 Steffen Klassert
` (3 preceding siblings ...)
2021-08-04 7:03 ` [PATCH 4/6] net/xfrm/compat: Copy xfrm_spdattr_type_t atributes Steffen Klassert
@ 2021-08-04 7:03 ` Steffen Klassert
2021-08-04 7:03 ` [PATCH 6/6] net: xfrm: Fix end of loop tests for list_for_each_entry Steffen Klassert
2021-08-04 10:10 ` pull request (net): ipsec 2021-08-04 patchwork-bot+netdevbpf
6 siblings, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2021-08-04 7:03 UTC (permalink / raw)
To: David Miller, Jakub Kicinski; +Cc: Herbert Xu, Steffen Klassert, netdev
From: Dmitry Safonov <dima@arista.com>
Set hthresh, dump it again and verify thresh.lbits && thresh.rbits.
They are passed as attributes of xfrm_spdattr_type_t, different from
other message attributes that use xfrm_attr_type_t.
Also, test attribute that is bigger than XFRMA_SPD_MAX, currently it
should be silently ignored.
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Dmitry Safonov <dima@arista.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
tools/testing/selftests/net/ipsec.c | 165 +++++++++++++++++++++++++++-
1 file changed, 163 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/ipsec.c b/tools/testing/selftests/net/ipsec.c
index f23438d512c5..3d7dde2c321b 100644
--- a/tools/testing/selftests/net/ipsec.c
+++ b/tools/testing/selftests/net/ipsec.c
@@ -484,13 +484,16 @@ enum desc_type {
MONITOR_ACQUIRE,
EXPIRE_STATE,
EXPIRE_POLICY,
+ SPDINFO_ATTRS,
};
const char *desc_name[] = {
"create tunnel",
"alloc spi",
"monitor acquire",
"expire state",
- "expire policy"
+ "expire policy",
+ "spdinfo attributes",
+ ""
};
struct xfrm_desc {
enum desc_type type;
@@ -1593,6 +1596,155 @@ static int xfrm_expire_policy(int xfrm_sock, uint32_t *seq,
return ret;
}
+static int xfrm_spdinfo_set_thresh(int xfrm_sock, uint32_t *seq,
+ unsigned thresh4_l, unsigned thresh4_r,
+ unsigned thresh6_l, unsigned thresh6_r,
+ bool add_bad_attr)
+
+{
+ struct {
+ struct nlmsghdr nh;
+ union {
+ uint32_t unused;
+ int error;
+ };
+ char attrbuf[MAX_PAYLOAD];
+ } req;
+ struct xfrmu_spdhthresh thresh;
+
+ memset(&req, 0, sizeof(req));
+ req.nh.nlmsg_len = NLMSG_LENGTH(sizeof(req.unused));
+ req.nh.nlmsg_type = XFRM_MSG_NEWSPDINFO;
+ req.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
+ req.nh.nlmsg_seq = (*seq)++;
+
+ thresh.lbits = thresh4_l;
+ thresh.rbits = thresh4_r;
+ if (rtattr_pack(&req.nh, sizeof(req), XFRMA_SPD_IPV4_HTHRESH, &thresh, sizeof(thresh)))
+ return -1;
+
+ thresh.lbits = thresh6_l;
+ thresh.rbits = thresh6_r;
+ if (rtattr_pack(&req.nh, sizeof(req), XFRMA_SPD_IPV6_HTHRESH, &thresh, sizeof(thresh)))
+ return -1;
+
+ if (add_bad_attr) {
+ BUILD_BUG_ON(XFRMA_IF_ID <= XFRMA_SPD_MAX + 1);
+ if (rtattr_pack(&req.nh, sizeof(req), XFRMA_IF_ID, NULL, 0)) {
+ pr_err("adding attribute failed: no space");
+ return -1;
+ }
+ }
+
+ if (send(xfrm_sock, &req, req.nh.nlmsg_len, 0) < 0) {
+ pr_err("send()");
+ return -1;
+ }
+
+ if (recv(xfrm_sock, &req, sizeof(req), 0) < 0) {
+ pr_err("recv()");
+ return -1;
+ } else if (req.nh.nlmsg_type != NLMSG_ERROR) {
+ printk("expected NLMSG_ERROR, got %d", (int)req.nh.nlmsg_type);
+ return -1;
+ }
+
+ if (req.error) {
+ printk("NLMSG_ERROR: %d: %s", req.error, strerror(-req.error));
+ return -1;
+ }
+
+ return 0;
+}
+
+static int xfrm_spdinfo_attrs(int xfrm_sock, uint32_t *seq)
+{
+ struct {
+ struct nlmsghdr nh;
+ union {
+ uint32_t unused;
+ int error;
+ };
+ char attrbuf[MAX_PAYLOAD];
+ } req;
+
+ if (xfrm_spdinfo_set_thresh(xfrm_sock, seq, 32, 31, 120, 16, false)) {
+ pr_err("Can't set SPD HTHRESH");
+ return KSFT_FAIL;
+ }
+
+ memset(&req, 0, sizeof(req));
+
+ req.nh.nlmsg_len = NLMSG_LENGTH(sizeof(req.unused));
+ req.nh.nlmsg_type = XFRM_MSG_GETSPDINFO;
+ req.nh.nlmsg_flags = NLM_F_REQUEST;
+ req.nh.nlmsg_seq = (*seq)++;
+ if (send(xfrm_sock, &req, req.nh.nlmsg_len, 0) < 0) {
+ pr_err("send()");
+ return KSFT_FAIL;
+ }
+
+ if (recv(xfrm_sock, &req, sizeof(req), 0) < 0) {
+ pr_err("recv()");
+ return KSFT_FAIL;
+ } else if (req.nh.nlmsg_type == XFRM_MSG_NEWSPDINFO) {
+ size_t len = NLMSG_PAYLOAD(&req.nh, sizeof(req.unused));
+ struct rtattr *attr = (void *)req.attrbuf;
+ int got_thresh = 0;
+
+ for (; RTA_OK(attr, len); attr = RTA_NEXT(attr, len)) {
+ if (attr->rta_type == XFRMA_SPD_IPV4_HTHRESH) {
+ struct xfrmu_spdhthresh *t = RTA_DATA(attr);
+
+ got_thresh++;
+ if (t->lbits != 32 || t->rbits != 31) {
+ pr_err("thresh differ: %u, %u",
+ t->lbits, t->rbits);
+ return KSFT_FAIL;
+ }
+ }
+ if (attr->rta_type == XFRMA_SPD_IPV6_HTHRESH) {
+ struct xfrmu_spdhthresh *t = RTA_DATA(attr);
+
+ got_thresh++;
+ if (t->lbits != 120 || t->rbits != 16) {
+ pr_err("thresh differ: %u, %u",
+ t->lbits, t->rbits);
+ return KSFT_FAIL;
+ }
+ }
+ }
+ if (got_thresh != 2) {
+ pr_err("only %d thresh returned by XFRM_MSG_GETSPDINFO", got_thresh);
+ return KSFT_FAIL;
+ }
+ } else if (req.nh.nlmsg_type != NLMSG_ERROR) {
+ printk("expected NLMSG_ERROR, got %d", (int)req.nh.nlmsg_type);
+ return KSFT_FAIL;
+ } else {
+ printk("NLMSG_ERROR: %d: %s", req.error, strerror(-req.error));
+ return -1;
+ }
+
+ /* Restore the default */
+ if (xfrm_spdinfo_set_thresh(xfrm_sock, seq, 32, 32, 128, 128, false)) {
+ pr_err("Can't restore SPD HTHRESH");
+ return KSFT_FAIL;
+ }
+
+ /*
+ * At this moment xfrm uses nlmsg_parse_deprecated(), which
+ * implies NL_VALIDATE_LIBERAL - ignoring attributes with
+ * (type > maxtype). nla_parse_depricated_strict() would enforce
+ * it. Or even stricter nla_parse().
+ * Right now it's not expected to fail, but to be ignored.
+ */
+ if (xfrm_spdinfo_set_thresh(xfrm_sock, seq, 32, 32, 128, 128, true))
+ return KSFT_PASS;
+
+ return KSFT_PASS;
+}
+
static int child_serv(int xfrm_sock, uint32_t *seq,
unsigned int nr, int cmd_fd, void *buf, struct xfrm_desc *desc)
{
@@ -1717,6 +1869,9 @@ static int child_f(unsigned int nr, int test_desc_fd, int cmd_fd, void *buf)
case EXPIRE_POLICY:
ret = xfrm_expire_policy(xfrm_sock, &seq, nr, &desc);
break;
+ case SPDINFO_ATTRS:
+ ret = xfrm_spdinfo_attrs(xfrm_sock, &seq);
+ break;
default:
printk("Unknown desc type %d", desc.type);
exit(KSFT_FAIL);
@@ -1994,8 +2149,10 @@ static int write_proto_plan(int fd, int proto)
* sizeof(xfrm_user_polexpire) = 168 | sizeof(xfrm_user_polexpire) = 176
*
* Check the affected by the UABI difference structures.
+ * Also, check translation for xfrm_set_spdinfo: it has it's own attributes
+ * which needs to be correctly copied, but not translated.
*/
-const unsigned int compat_plan = 4;
+const unsigned int compat_plan = 5;
static int write_compat_struct_tests(int test_desc_fd)
{
struct xfrm_desc desc = {};
@@ -2019,6 +2176,10 @@ static int write_compat_struct_tests(int test_desc_fd)
if (__write_desc(test_desc_fd, &desc))
return -1;
+ desc.type = SPDINFO_ATTRS;
+ if (__write_desc(test_desc_fd, &desc))
+ return -1;
+
return 0;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 6/6] net: xfrm: Fix end of loop tests for list_for_each_entry
2021-08-04 7:03 pull request (net): ipsec 2021-08-04 Steffen Klassert
` (4 preceding siblings ...)
2021-08-04 7:03 ` [PATCH 5/6] selftests/net/ipsec: Add test for xfrm_spdattr_type_t Steffen Klassert
@ 2021-08-04 7:03 ` Steffen Klassert
2021-08-04 10:10 ` pull request (net): ipsec 2021-08-04 patchwork-bot+netdevbpf
6 siblings, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2021-08-04 7:03 UTC (permalink / raw)
To: David Miller, Jakub Kicinski; +Cc: Herbert Xu, Steffen Klassert, netdev
From: Harshvardhan Jha <harshvardhan.jha@oracle.com>
The list_for_each_entry() iterator, "pos" in this code, can never be
NULL so the warning will never be printed.
Signed-off-by: Harshvardhan Jha <harshvardhan.jha@oracle.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/xfrm/xfrm_ipcomp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index 2e8afe078d61..cb40ff0ff28d 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -241,7 +241,7 @@ static void ipcomp_free_tfms(struct crypto_comp * __percpu *tfms)
break;
}
- WARN_ON(!pos);
+ WARN_ON(list_entry_is_head(pos, &ipcomp_tfms_list, list));
if (--pos->users)
return;
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/6] net: xfrm: fix memory leak in xfrm_user_rcv_msg
2021-08-04 7:03 ` [PATCH 1/6] net: xfrm: fix memory leak in xfrm_user_rcv_msg Steffen Klassert
@ 2021-08-04 10:10 ` patchwork-bot+netdevbpf
0 siblings, 0 replies; 9+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-08-04 10:10 UTC (permalink / raw)
To: Steffen Klassert; +Cc: davem, kuba, herbert, netdev
Hello:
This series was applied to netdev/net.git (refs/heads/master):
On Wed, 4 Aug 2021 09:03:24 +0200 you wrote:
> From: Pavel Skripkin <paskripkin@gmail.com>
>
> Syzbot reported memory leak in xfrm_user_rcv_msg(). The
> problem was is non-freed skb's frag_list.
>
> In skb_release_all() skb_release_data() will be called only
> in case of skb->head != NULL, but netlink_skb_destructor()
> sets head to NULL. So, allocated frag_list skb should be
> freed manualy, since consume_skb() won't take care of it
>
> [...]
Here is the summary with links:
- [1/6] net: xfrm: fix memory leak in xfrm_user_rcv_msg
https://git.kernel.org/netdev/net/c/7c1a80e80cde
- [2/6] Revert "xfrm: policy: Read seqcount outside of rcu-read side in xfrm_policy_lookup_bytype"
https://git.kernel.org/netdev/net/c/eaf228263921
- [3/6] xfrm: Fix RCU vs hash_resize_mutex lock inversion
https://git.kernel.org/netdev/net/c/2580d3f40022
- [4/6] net/xfrm/compat: Copy xfrm_spdattr_type_t atributes
https://git.kernel.org/netdev/net/c/4e9505064f58
- [5/6] selftests/net/ipsec: Add test for xfrm_spdattr_type_t
https://git.kernel.org/netdev/net/c/70bfdf62e93a
- [6/6] net: xfrm: Fix end of loop tests for list_for_each_entry
https://git.kernel.org/netdev/net/c/480e93e12aa0
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: pull request (net): ipsec 2021-08-04
2021-08-04 7:03 pull request (net): ipsec 2021-08-04 Steffen Klassert
` (5 preceding siblings ...)
2021-08-04 7:03 ` [PATCH 6/6] net: xfrm: Fix end of loop tests for list_for_each_entry Steffen Klassert
@ 2021-08-04 10:10 ` patchwork-bot+netdevbpf
6 siblings, 0 replies; 9+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-08-04 10:10 UTC (permalink / raw)
To: Steffen Klassert; +Cc: davem, kuba, herbert, netdev
Hello:
This pull request was applied to netdev/net.git (refs/heads/master):
On Wed, 4 Aug 2021 09:03:23 +0200 you wrote:
> 1) Fix a sysbot reported memory leak in xfrm_user_rcv_msg.
> From Pavel Skripkin.
>
> 2) Revert "xfrm: policy: Read seqcount outside of rcu-read side
> in xfrm_policy_lookup_bytype". This commit tried to fix a
> lockin bug, but only cured some of the symptoms. A proper
> fix is applied on top of this revert.
>
> [...]
Here is the summary with links:
- pull request (net): ipsec 2021-08-04
https://git.kernel.org/netdev/net/c/d00551b40201
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-08-04 10:11 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-04 7:03 pull request (net): ipsec 2021-08-04 Steffen Klassert
2021-08-04 7:03 ` [PATCH 1/6] net: xfrm: fix memory leak in xfrm_user_rcv_msg Steffen Klassert
2021-08-04 10:10 ` patchwork-bot+netdevbpf
2021-08-04 7:03 ` [PATCH 2/6] Revert "xfrm: policy: Read seqcount outside of rcu-read side in xfrm_policy_lookup_bytype" Steffen Klassert
2021-08-04 7:03 ` [PATCH 3/6] xfrm: Fix RCU vs hash_resize_mutex lock inversion Steffen Klassert
2021-08-04 7:03 ` [PATCH 4/6] net/xfrm/compat: Copy xfrm_spdattr_type_t atributes Steffen Klassert
2021-08-04 7:03 ` [PATCH 5/6] selftests/net/ipsec: Add test for xfrm_spdattr_type_t Steffen Klassert
2021-08-04 7:03 ` [PATCH 6/6] net: xfrm: Fix end of loop tests for list_for_each_entry Steffen Klassert
2021-08-04 10:10 ` pull request (net): ipsec 2021-08-04 patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.