Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683579 > unrolled thread

[GIT] Networking

Back to article view | Back to linux.kernel

Contents

  [GIT] Networking David Miller <davem@davemloft.net> - 2017-07-08 12:40 +0200
    Re: [GIT] Networking Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-09 21:00 +0200
      Re: [GIT] Networking Sowmini Varadhan <sowmini.varadhan@oracle.com> - 2017-07-09 21:20 +0200
        Re: [GIT] Networking David Miller <davem@davemloft.net> - 2017-07-09 22:50 +0200
          Re: [GIT] Networking Herbert Xu <herbert@gondor.apana.org.au> - 2017-07-10 12:10 +0200
            Re: [GIT] Networking Sowmini Varadhan <sowmini.varadhan@oracle.com> - 2017-07-10 14:20 +0200
              Re: [GIT] Networking Herbert Xu <herbert@gondor.apana.org.au> - 2017-07-10 16:10 +0200
                Re: [GIT] Networking David Miller <davem@davemloft.net> - 2017-07-11 22:40 +0200
                  Re: [GIT] Networking Herbert Xu <herbert@gondor.apana.org.au> - 2017-07-12 03:50 +0200

#1683579 — [GIT] Networking

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

[toc] | [next] | [standalone]

#1683823

On Sat, Jul 8, 2017 at 3:36 AM, David Miller <davem@davemloft.net> 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 ]---

[toc] | [prev] | [next] | [standalone]

#1683825

On (07/09/17 11:49), Linus Torvalds wrote:
> 
> On Sat, Jul 8, 2017 at 3:36 AM, David Miller <davem@davemloft.net> 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 ]---

[toc] | [prev] | [next] | [standalone]

#1683839

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
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 <davem@davemloft.net> 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.

[toc] | [prev] | [next] | [standalone]

#1684108

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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[toc] | [prev] | [next] | [standalone]

#1684181

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

[toc] | [prev] | [next] | [standalone]

#1684273

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 <torvalds@linux-foundation.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[toc] | [prev] | [next] | [standalone]

#1685330

From: Herbert Xu <herbert@gondor.apana.org.au>
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 <torvalds@linux-foundation.org>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: David S. Miller <davem@davemloft.net>

Looks good, is this going via my tree or your's?

[toc] | [prev] | [next] | [standalone]

#1685450

On Tue, Jul 11, 2017 at 01:31:14PM -0700, David Miller wrote:
>
> Acked-by: David S. Miller <davem@davemloft.net>
> 
> Looks good, is this going via my tree or your's?

I'll push it along.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web