Mostly fixing some light fallout from the changes that went into
the merge window.
1) Fix memory leaks on network namespace teardown in netfilter, from
Liping Zhang.
2) When comparing ipv6 nexthops, we have to take the lightweight tunnel
state into account as well. From David Ahern.
3) Fix socket option object length check in the new TLS code, from
Matthias Rosenfelder.
4) Fix memory leak in nfp driver flower support, from Jakub Kicinski.
5) Several netlink attribute validation fixes in cfg80211, from Srinivas
Dasari.
6) Fix context array leak in virtio_net, from Jason Wang.
7) SKB use after free in hns driver, from Yusheng Lin.
8) Fix socket leak on accept() in RDS, from Sowmini Varadhan. Also
add a WARN_ON() to sock_graft() so other protocol stacks don't trip
over this as well.
Please pull, thanks a lot!
The following changes since commit 9b51f04424e17051a89ab32d892ca66b2a104825:
Merge branch 'parisc-4.13-2' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux (2017-07-05 17:41:31 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git
for you to fetch changes up to e8df760307830ca26cf380a9a4b36468a0352fa5:
net: ethernet: mediatek: remove useless code in mtk_probe() (2017-07-08 11:27:55 +0100)
----------------------------------------------------------------
Christophe Jaillet (1):
arcnet: com20020-pci: Fix an error handling path in 'com20020pci_probe()'
David Ahern (1):
net: ipv6: Compare lwstate in detecting duplicate nexthops
David S. Miller (5):
Merge git://git.kernel.org/.../pablo/nf
Merge tag 'mac80211-for-davem-2017-07-07' of git://git.kernel.org/.../jberg/mac80211
net: Update networking MAINTAINERS entry.
Merge branch 'hns-fixes'
Merge branch 'rds-tcp-sock_graft-leak'
Derek Chickles (1):
liquidio: fix bug in soft reset failure detection
Geert Uytterhoeven (1):
ptp: dte: Use LL suffix for 64-bit constants
Gustavo A. R. Silva (1):
net: ethernet: mediatek: remove useless code in mtk_probe()
Jakub Kicinski (1):
nfp: flower: add missing clean up call to avoid memory leaks
Jason Wang (1):
virtio-net: fix leaking of ctx array
Liping Zhang (2):
netfilter: nf_ct_dccp/sctp: fix memory leak after netns cleanup
netfilter: ebt_nflog: fix unexpected truncated packet
Matthias Rosenfelder (1):
TLS: Fix length check in do_tls_getsockopt_tx()
Nicolas Dichtel (1):
doc: SKB_GSO_[IPIP|SIT] have been replaced
Nikolay Aleksandrov (1):
vrf: fix bug_on triggered by rx when destroying a vrf
Roopa Prabhu (1):
mpls: fix uninitialized in_label var warning in mpls_getroute
Sowmini Varadhan (2):
rds: tcp: use sock_create_lite() to create the accept socket
net/sock: add WARN_ON(parent->sk) in sock_graft()
Srinivas Dasari (4):
cfg80211: Check if PMKID attribute is of expected size
cfg80211: Check if NAN service ID is of expected size
cfg80211: Define nla_policy for NL80211_ATTR_LOCAL_MESH_POWER_MODE
cfg80211: Validate frequencies nested in NL80211_ATTR_SCAN_FREQUENCIES
WANG Cong (1):
bonding: avoid NETDEV_CHANGEMTU event when unregistering slave
Wu Fengguang (1):
tcp: md5: tcp_md5_do_lookup_exact() can be static
Yunsheng Lin (2):
net: hns: Fix a wrong op phy C45 code
net: hns: Fix a skb used after free bug
Zheng Li (1):
sctp: set the value of flowi6_oif to sk_bound_dev_if to make sctp_v6_get_dst to find the correct route entry.
vishnuvardhan (1):
net: macb: Adding Support for Jumbo Frames up to 10240 Bytes in SAMA5D3
Documentation/networking/segmentation-offloads.txt | 2 +-
MAINTAINERS | 2 --
drivers/net/arcnet/com20020-pci.c | 6 ++++--
drivers/net/bonding/bond_main.c | 15 +++++++++------
drivers/net/ethernet/cadence/macb_main.c | 3 ++-
drivers/net/ethernet/cavium/liquidio/cn23xx_pf_device.c | 2 +-
drivers/net/ethernet/cavium/liquidio/cn66xx_device.c | 2 +-
drivers/net/ethernet/hisilicon/hns/hns_enet.c | 22 ++++++++++------------
drivers/net/ethernet/hisilicon/hns/hns_enet.h | 6 +++---
drivers/net/ethernet/hisilicon/hns_mdio.c | 2 +-
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 5 -----
drivers/net/ethernet/netronome/nfp/flower/main.c | 1 +
drivers/net/virtio_net.c | 1 +
drivers/net/vrf.c | 11 ++++++-----
drivers/ptp/ptp_dte.c | 2 +-
include/linux/netdevice.h | 1 +
include/net/ip6_route.h | 8 ++++++++
include/net/sock.h | 1 +
net/bridge/netfilter/ebt_nflog.c | 1 +
net/core/dev.c | 3 ++-
net/ipv4/tcp_ipv4.c | 6 +++---
net/ipv6/ip6_fib.c | 5 +----
net/ipv6/route.c | 8 +-------
net/mpls/af_mpls.c | 12 ++++++++----
net/netfilter/nf_conntrack_proto_dccp.c | 7 +++++++
net/netfilter/nf_conntrack_proto_sctp.c | 7 +++++++
net/rds/tcp_listen.c | 2 +-
net/sctp/ipv6.c | 2 ++
net/tls/tls_main.c | 2 +-
net/wireless/nl80211.c | 10 +++++++---
30 files changed, 92 insertions(+), 65 deletions(-)
On Sat, Jul 8, 2017 at 3:36 AM, David Miller <[email protected]> wrote:
>
> 8) Fix socket leak on accept() in RDS, from Sowmini Varadhan. Also
> add a WARN_ON() to sock_graft() so other protocol stacks don't trip
> over this as well.
Hmm. This one triggers for me on both my desktop and laptop at bootup.
Bog-standard machines, running F25 and F24 respectively.
The warning doesn't seem particularly useful, although maybe that
"alg_accept()" gives people who know this code enough of a clue.
Linus
------------[ cut here ]------------
WARNING: CPU: 1 PID: 492 at ./include/net/sock.h:1700 af_alg_accept+0x1bf/0x1f0
CPU: 1 PID: 492 Comm: systemd-cryptse Not tainted 4.12.0-09010-g2b976203417c #1
Hardware name: System manufacturer System Product Name/Z170-K, BIOS
1803 05/06/2016
RIP: 0010:af_alg_accept+0x1bf/0x1f0
Call Trace:
alg_accept+0x15/0x20
SYSC_accept4+0x105/0x210
? getnstimeofday64+0xe/0x20
? __audit_syscall_entry+0xb1/0xf0
? syscall_trace_enter+0x1bd/0x2d0
? __audit_syscall_exit+0x1a5/0x2a0
SyS_accept+0x10/0x20
do_syscall_64+0x61/0x140
entry_SYSCALL64_slow_path+0x25/0x25
---[ end trace a35e5baea85df269 ]---
On (07/09/17 11:49), Linus Torvalds wrote:
>
> On Sat, Jul 8, 2017 at 3:36 AM, David Miller <[email protected]> wrote:
> >
> > 8) Fix socket leak on accept() in RDS, from Sowmini Varadhan. Also
> > add a WARN_ON() to sock_graft() so other protocol stacks don't trip
> > over this as well.
>
> Hmm. This one triggers for me on both my desktop and laptop at bootup.
> Bog-standard machines, running F25 and F24 respectively.
>
> The warning doesn't seem particularly useful, although maybe that
> "alg_accept()" gives people who know this code enough of a clue.
My initial question was whether sock_graft() should do a sock_put()
before cutting loose the existing parent->sk and assigning a new
parent->sk (https://www.spinics.net/lists/netdev/msg442191.html)
It look like PF_ALG sets up a ->sk in alg_create() (but this
would get over-written in alg_accept()?)
Cc'ing Herbert to see if this is expected behavior (and PF_ALG
somehow does the right thing with the refcount for the ->sk
set up in alg_create) in which case I suppose we should drop the
WARN_ON.
--Sowmini
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 492 at ./include/net/sock.h:1700 af_alg_accept+0x1bf/0x1f0
> CPU: 1 PID: 492 Comm: systemd-cryptse Not tainted 4.12.0-09010-g2b976203417c #1
> Hardware name: System manufacturer System Product Name/Z170-K, BIOS
> 1803 05/06/2016
> RIP: 0010:af_alg_accept+0x1bf/0x1f0
> Call Trace:
> alg_accept+0x15/0x20
> SYSC_accept4+0x105/0x210
> ? getnstimeofday64+0xe/0x20
> ? __audit_syscall_entry+0xb1/0xf0
> ? syscall_trace_enter+0x1bd/0x2d0
> ? __audit_syscall_exit+0x1a5/0x2a0
> SyS_accept+0x10/0x20
> do_syscall_64+0x61/0x140
> entry_SYSCALL64_slow_path+0x25/0x25
> ---[ end trace a35e5baea85df269 ]---
From: Sowmini Varadhan <[email protected]>
Date: Sun, 9 Jul 2017 15:11:31 -0400
> On (07/09/17 11:49), Linus Torvalds wrote:
>>
>> On Sat, Jul 8, 2017 at 3:36 AM, David Miller <[email protected]> wrote:
>> >
>> > 8) Fix socket leak on accept() in RDS, from Sowmini Varadhan. Also
>> > add a WARN_ON() to sock_graft() so other protocol stacks don't trip
>> > over this as well.
>>
>> Hmm. This one triggers for me on both my desktop and laptop at bootup.
>> Bog-standard machines, running F25 and F24 respectively.
>>
>> The warning doesn't seem particularly useful, although maybe that
>> "alg_accept()" gives people who know this code enough of a clue.
>
> My initial question was whether sock_graft() should do a sock_put()
> before cutting loose the existing parent->sk and assigning a new
> parent->sk (https://www.spinics.net/lists/netdev/msg442191.html)
>
> It look like PF_ALG sets up a ->sk in alg_create() (but this
> would get over-written in alg_accept()?)
>
> Cc'ing Herbert to see if this is expected behavior (and PF_ALG
> somehow does the right thing with the refcount for the ->sk
> set up in alg_create) in which case I suppose we should drop the
> WARN_ON.
I think we've found yet another socket leak, this time in
AF_ALG.
On Sun, Jul 09, 2017 at 09:40:43PM +0100, David Miller wrote:
>
> > It look like PF_ALG sets up a ->sk in alg_create() (but this
> > would get over-written in alg_accept()?)
No it does not. The struct socket comes from sys_accept() and
AFAICS it doesn't carry a struck sock with it.
> > Cc'ing Herbert to see if this is expected behavior (and PF_ALG
> > somehow does the right thing with the refcount for the ->sk
> > set up in alg_create) in which case I suppose we should drop the
> > WARN_ON.
>
> I think we've found yet another socket leak, this time in
> AF_ALG.
Hmm, I can't see the problem in af_alg_accept. The struct socket
comes directly from sys_accept() which creates it using sock_alloc.
So the only thing I can think of is that the memory returned by
sock_alloc is not zeroed and therefore the WARN_ON is just reading
garbage.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On (07/10/17 18:05), Herbert Xu wrote:
>
> Hmm, I can't see the problem in af_alg_accept. The struct socket
> comes directly from sys_accept() which creates it using sock_alloc.
>
> So the only thing I can think of is that the memory returned by
> sock_alloc is not zeroed and therefore the WARN_ON is just reading
> garbage.
Then it is odd that this WARN_ON is not triggered for other sockets
(e.g., for TCP sockets), though it happens easily with AF_ALG.
But it's not sock_alloc() - that function is returning a properly
zeroed ->sk.
The reason that the WARN_ON is triggered is that af_alg_accept() calls
sock_init_data() which does
2636 if (sock) {
:
2639 sock->sk = sk;
So we can do one of the following:
1. drop the WARN_ON(), which makes true leaks hard to detect
2. change the WARN_ON() to WARN_ON(parent->sk && parent->sk != sk)
#2 assumes that all the refcount book-keeping is being done
correctly (there is the danger that we end up taking 2 refs on the sk)
--Sowmini
On Mon, Jul 10, 2017 at 08:10:02AM -0400, Sowmini Varadhan wrote:
>
> The reason that the WARN_ON is triggered is that af_alg_accept() calls
> sock_init_data() which does
>
> 2636 if (sock) {
> :
> 2639 sock->sk = sk;
Oh yes. This started out with just sock_init_data which would
not have triggered your warning. Then someone came along and
added sock_graft which basically duplicates work already done in
sock_init_data.
In fact the reason they wanted sock_graft was purely to call
security_sock_graft to initialise some SELinux state. So we
could avoid your warning by calling that directly.
---8<---
crypto: af_alg - Avoid sock_graft call warning
The newly added sock_graft warning triggers in af_alg_accept.
It's harmless as we're essentially doing sock->sk = sock->sk.
The sock_graft call is actually redundant because all the work
it does is subsumed by sock_init_data. However, it was added
to placate SELinux as it uses it to initialise its internal state.
This patch avoisd the warning by making the SELinux call directly.
Reported-by: Linus Torvalds <[email protected]>
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 3556d8e..92a3d54 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -287,7 +287,7 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
goto unlock;
sock_init_data(newsock, sk2);
- sock_graft(sk2, newsock);
+ security_sock_graft(sk2, newsock);
security_sk_clone(sk, sk2);
err = type->accept(ask->private, sk2);
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: Herbert Xu <[email protected]>
Date: Mon, 10 Jul 2017 22:00:48 +0800
> crypto: af_alg - Avoid sock_graft call warning
>
> The newly added sock_graft warning triggers in af_alg_accept.
> It's harmless as we're essentially doing sock->sk = sock->sk.
>
> The sock_graft call is actually redundant because all the work
> it does is subsumed by sock_init_data. However, it was added
> to placate SELinux as it uses it to initialise its internal state.
>
> This patch avoisd the warning by making the SELinux call directly.
>
> Reported-by: Linus Torvalds <[email protected]>
> Signed-off-by: Herbert Xu <[email protected]>
Acked-by: David S. Miller <[email protected]>
Looks good, is this going via my tree or your's?
On Tue, Jul 11, 2017 at 01:31:14PM -0700, David Miller wrote:
>
> Acked-by: David S. Miller <[email protected]>
>
> Looks good, is this going via my tree or your's?
I'll push it along. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt