Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net] Revert "mdio_bus: Remove unneeded gpiod NULL check"
From: David Miller @ 2017-09-09  4:13 UTC (permalink / raw)
  To: f.fainelli
  Cc: netdev, linus.walleij, andrew, festevam, sergei.shtylyov,
	fabio.estevam, Bryan.Whitehead
In-Reply-To: <20170908223807.6015-1-f.fainelli@gmail.com>

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri,  8 Sep 2017 15:38:07 -0700

> This reverts commit 95b80bf3db03c2bf572a357cf74b9a6aefef0a4a ("mdio_bus:
> Remove unneeded gpiod NULL check"), this commit assumed that GPIOLIB
> checks for NULL descriptors, so it's safe to drop them, but it is not
> when CONFIG_GPIOLIB is disabled in the kernel. If we do call
> gpiod_set_value_cansleep() on a GPIO descriptor we will issue warnings
> coming from the inline stubs declared in include/linux/gpio/consumer.h.
> 
> Fixes: 95b80bf3db03 ("mdio_bus: Remove unneeded gpiod NULL check")
> Reported-by: Woojung Huh <Woojung.Huh@microchip.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Applied.

^ permalink raw reply

* Re: [PATCH net] bpf: make error reporting in bpf_warn_invalid_xdp_action more clear
From: David Miller @ 2017-09-09  4:13 UTC (permalink / raw)
  To: daniel; +Cc: ast, john.fastabend, netdev
In-Reply-To: <a89dbb43f7196f6ba8c5fd1f07c7e4bcc97726c3.1504913751.git.daniel@iogearbox.net>

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Sat,  9 Sep 2017 01:40:35 +0200

> Differ between illegal XDP action code and just driver
> unsupported one to provide better feedback when we throw
> a one-time warning here. Reason is that with 814abfabef3c
> ("xdp: add bpf_redirect helper function") not all drivers
> support the new XDP return code yet and thus they will
> fall into their 'default' case when checking for return
> codes after program return, which then triggers a
> bpf_warn_invalid_xdp_action() stating that the return
> code is illegal, but from XDP perspective it's not.
> 
> I decided not to place something like a XDP_ACT_MAX define
> into uapi i) given we don't have this either for all other
> program types, ii) future action codes could have further
> encoding there, which would render such define unsuitable
> and we wouldn't be able to rip it out again, and iii) we
> rarely add new action codes.
> 
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Acked-by: Alexei Starovoitov <ast@kernel.org>

Applied.

^ permalink raw reply

* Re: [PATCH] ipv4: Namespaceify tcp_max_orphans knob
From: Cong Wang @ 2017-09-09  4:35 UTC (permalink / raw)
  To: 严海双
  Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
	Eric Dumazet, Linux Kernel Network Developers, LKML
In-Reply-To: <798CA25A-CA09-4D06-A9B6-7C5791A6EEC1@cmss.chinamobile.com>

On Fri, Sep 8, 2017 at 6:25 PM, 严海双 <yanhaishuang@cmss.chinamobile.com> wrote:
>
>
>> On 2017年9月9日, at 上午6:13, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>
>> On Wed, Sep 6, 2017 at 8:10 PM, Haishuang Yan
>> <yanhaishuang@cmss.chinamobile.com> wrote:
>>> Different namespace application might require different maximal number
>>> of TCP sockets independently of the host.
>>
>> So after your patch we could have N * net->ipv4.sysctl_tcp_max_orphans
>> in a whole system, right? This just makes OOM easier to trigger.
>>
>
> From my understanding, before the patch, we had N * net->ipv4.sysctl_tcp_max_orphans,
> and after the patch, we could have ns1.sysctl_tcp_max_orphans + ns2.sysctl_tcp_max_orphans
> + ns3.sysctl_tcp_max_orphans, is that right? Thanks for your reviewing.

Nope, by N I mean the number of containers. Before your patch, the limit
is global, after your patch it is per container.

^ permalink raw reply

* [GIT] Networking
From: David Miller @ 2017-09-09  5:06 UTC (permalink / raw)
  To: torvalds; +Cc: akpm, netdev, linux-kernel


The iwlwifi firmware compat fix is in here as well as some other
stuff:

1) Fix request socket leak introduced by BPF deadlock fix, from Eric
   Dumazet.

2) Fix VLAN handling with TXQs in mac80211, from Johannes Berg.

3) Missing __qdisc_drop conversions in prio and qfq schedulers, from
   Gao Feng.

4) Use after free in netlink nlk groups handling, from Xin Long.

5) Handle MTU update properly in ipv6 gre tunnels, from Xin Long.

6) Fix leak of ipv6 fib tables on netns teardown, from Sabrina Dubroca
   with follow-on fix from Eric Dumazet.

7) Need RCU and preemption disabled during generic XDP data patch, from
   John Fastabend.

Please pull, thanks a lot!

The following changes since commit 80cee03bf1d626db0278271b505d7f5febb37bba:

  Merge branch 'linus' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6 (2017-09-06 15:17:17 -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 9beb8bedb05c5f3a353dba62b8fa7cbbb9ec685e:

  bpf: make error reporting in bpf_warn_invalid_xdp_action more clear (2017-09-08 21:13:09 -0700)

----------------------------------------------------------------
Arnd Bergmann (1):
      isdn: isdnloop: fix logic error in isdnloop_sendbuf

Avraham Stern (1):
      mac80211: flush hw_roc_start work before cancelling the ROC

Baruch Siach (5):
      dt-binding: phy: don't confuse with Ethernet phy properties
      dt-bindings: net: don't confuse with generic PHY property
      dt-bindings: add SFF vendor prefix
      dt-binding: net: sfp binding documentation
      net: phy: sfp: rename dt properties to match the binding

Beni Lev (1):
      mac80211_hwsim: Use proper TX power

Chunho Lee (1):
      mac80211: Fix null pointer dereference with iTXQ support

Daniel Borkmann (2):
      bpf: don't select potentially stale ri->map from buggy xdp progs
      bpf: make error reporting in bpf_warn_invalid_xdp_action more clear

David S. Miller (4):
      Merge tag 'mac80211-for-davem-2017-09-07' of git://git.kernel.org/.../jberg/mac80211
      Merge tag 'wireless-drivers-for-davem-2017-09-08' of git://git.kernel.org/.../kvalo/wireless-drivers
      Merge git://git.kernel.org/.../pablo/nf
      Merge branch 'xdp-bpf-fixes'

Emmanuel Grumbach (1):
      cfg80211: honor NL80211_RRF_NO_HT40{MINUS,PLUS}

Eric Dumazet (2):
      tcp: fix a request socket leak
      ipv6: fix typo in fib6_net_exit()

Florian Fainelli (1):
      Revert "mdio_bus: Remove unneeded gpiod NULL check"

Florian Westphal (5):
      netfilter: nf_nat: don't bug when mapping already exists
      netfilter: xtables: add scheduling opportunity in get_counters
      netfilter: nat: Revert "netfilter: nat: convert nat bysrc hash to rhashtable"
      netfilter: nat: use keyed locks
      netfilter: core: remove erroneous warn_on

Gao Feng (1):
      sched: Use __qdisc_drop instead of kfree_skb in sch_prio and sch_qfq

Haishuang Yan (2):
      ip_tunnel: fix setting ttl and tos value in collect_md mode
      ip6_tunnel: fix setting hop_limit value for ipv6 tunnel

Håkon Bugge (1):
      rds: Fix incorrect statistics counting

Ian W MORRISON (1):
      brcmfmac: feature check for multi-scheduled scan fails on bcm4345 devices

Igor Mitsyanko (1):
      nl80211: look for HT/VHT capabilities in beacon's tail

Ilan peer (1):
      mac80211: Complete ampdu work schedule during session tear down

Ivan Khoronzhuk (1):
      net: ethernet: ti: netcp_core: no need in netif_napi_del

Jiri Pirko (1):
      net: sched: fix memleak for chain zero

Johannes Berg (3):
      mac80211: fix VLAN handling with TXQs
      mac80211: agg-tx: call drv_wake_tx_queue in proper context
      mac80211: fix deadlock in driver-managed RX BA session start

John Fastabend (3):
      net: rcu lock and preempt disable missing around generic xdp
      bpf: add support for sockmap detach programs
      bpf: devmap, use cond_resched instead of cpu_relax

Kees Cook (1):
      net: tulip: Constify tulip_tbl

Kleber Sacilotto de Souza (1):
      tipc: remove unnecessary call to dev_net()

Larry Finger (2):
      rtlwifi: btcoexist: Fix breakage of ant_sel for rtl8723be
      rtlwifi: btcoexist: Fix antenna selection code

Liad Kaufman (1):
      mac80211: add MESH IE in the correct order

Luca Coelho (1):
      iwlwifi: mvm: only send LEDS_CMD when the FW supports it

Marcelo Ricardo Leitner (1):
      sctp: fix missing wake ups in some situations

Mathieu Malaterre (1):
      davicom: Display proper debug level up to 6

Paolo Abeni (1):
      udp: drop head states only when all skb references are gone

Sabrina Dubroca (1):
      ipv6: fix memory leak with multiple tables during netns destruction

Sharon Dvir (1):
      mac80211: shorten debug prints using ht_dbg() to avoid warning

Simon Dinkin (1):
      mac80211: fix incorrect assignment of reassoc value

Vishwanath Pai (1):
      netfilter: xt_hashlimit: fix build error caused by 64bit division

Xin Long (5):
      netlink: fix an use-after-free issue for nlk groups
      netlink: access nlk groups safely in netlink bind and getname
      ip6_gre: update mtu properly in ip6gre_err
      netfilter: ipvs: fix the issue that sctp_conn_schedule drops non-INIT packet
      netfilter: ipvs: do not create conn for ABORT packet in sctp_conn_schedule

Zhizhou Tian (1):
      netfilter: xt_hashlimit: alloc hashtable with right size

 Documentation/devicetree/bindings/net/ethernet.txt               |   4 +++
 Documentation/devicetree/bindings/net/sff,sfp.txt                |  76 ++++++++++++++++++++++++++++++++++++++++++++++
 Documentation/devicetree/bindings/phy/phy-bindings.txt           |   4 ++-
 Documentation/devicetree/bindings/vendor-prefixes.txt            |   1 +
 drivers/isdn/isdnloop/isdnloop.c                                 |   2 +-
 drivers/net/ethernet/davicom/dm9000.c                            |   2 +-
 drivers/net/ethernet/dec/tulip/tulip.h                           |   2 +-
 drivers/net/ethernet/dec/tulip/tulip_core.c                      |   2 +-
 drivers/net/ethernet/ti/netcp_core.c                             |   1 -
 drivers/net/phy/mdio_bus.c                                       |   6 ++--
 drivers/net/phy/sfp.c                                            |   4 +--
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c       |   3 +-
 drivers/net/wireless/intel/iwlwifi/fw/file.h                     |   1 +
 drivers/net/wireless/intel/iwlwifi/mvm/led.c                     |   3 +-
 drivers/net/wireless/mac80211_hwsim.c                            |   2 --
 drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c |   5 +++-
 drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c    |  23 +++++++++-----
 include/linux/bpf.h                                              |   8 ++---
 include/linux/skbuff.h                                           |   2 +-
 include/net/mac80211.h                                           |  15 ++--------
 include/net/netfilter/nf_conntrack.h                             |   3 +-
 include/net/netfilter/nf_nat.h                                   |   1 -
 include/uapi/linux/bpf.h                                         |   4 +--
 kernel/bpf/devmap.c                                              |   2 +-
 kernel/bpf/sockmap.c                                             |   2 +-
 kernel/bpf/syscall.c                                             |  27 ++++++++++-------
 kernel/bpf/verifier.c                                            |  16 ++++++++++
 net/core/dev.c                                                   |  25 ++++++++++------
 net/core/filter.c                                                |  27 +++++++++++++++--
 net/core/skbuff.c                                                |   9 ++----
 net/ipv4/ip_tunnel.c                                             |   4 +--
 net/ipv4/netfilter/arp_tables.c                                  |   1 +
 net/ipv4/netfilter/ip_tables.c                                   |   1 +
 net/ipv4/tcp_ipv4.c                                              |   6 ++--
 net/ipv4/udp.c                                                   |   5 +++-
 net/ipv6/ip6_fib.c                                               |  25 ++++++++++++----
 net/ipv6/ip6_gre.c                                               |   4 ++-
 net/ipv6/ip6_tunnel.c                                            |   1 +
 net/ipv6/netfilter/ip6_tables.c                                  |   1 +
 net/ipv6/tcp_ipv6.c                                              |   6 ++--
 net/mac80211/agg-rx.c                                            |  32 +++++++++++++-------
 net/mac80211/agg-tx.c                                            |   8 +++--
 net/mac80211/ht.c                                                |  24 +++++++++++++--
 net/mac80211/ieee80211_i.h                                       |   4 +++
 net/mac80211/iface.c                                             |  20 +++++++++++--
 net/mac80211/mlme.c                                              |   2 +-
 net/mac80211/offchannel.c                                        |   2 ++
 net/mac80211/tx.c                                                |  36 +++++++++++++++++-----
 net/mac80211/util.c                                              |   2 +-
 net/netfilter/core.c                                             |   2 +-
 net/netfilter/ipvs/ip_vs_proto_sctp.c                            |   8 +++--
 net/netfilter/nf_nat_core.c                                      | 146 +++++++++++++++++++++++++++++++++++++++++------------------------------------------------
 net/netfilter/xt_hashlimit.c                                     |  16 +++++-----
 net/netlink/af_netlink.c                                         |  22 +++++++++-----
 net/rds/send.c                                                   |  10 +++++--
 net/sched/cls_api.c                                              |  18 +++++------
 net/sched/sch_prio.c                                             |   2 +-
 net/sched/sch_qfq.c                                              |   2 +-
 net/sctp/ulpqueue.c                                              |   3 +-
 net/tipc/bearer.c                                                |   2 +-
 net/wireless/nl80211.c                                           |   4 +--
 net/wireless/reg.c                                               |  20 +++++++++++--
 tools/testing/selftests/bpf/test_maps.c                          |  51 ++++++++++++++++++++++++++++++-
 63 files changed, 537 insertions(+), 235 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/sff,sfp.txt

^ permalink raw reply

* Re: [PATCH net-next 1/1] sched: Use __qdisc_drop instead of kfree_skb in sch_prio and sch_qfq
From: David Miller @ 2017-09-07  4:20 UTC (permalink / raw)
  To: gfree.wind; +Cc: jhs, xiyou.wangcong, jiri, edumazet, netdev
In-Reply-To: <1504506072-46207-1-git-send-email-gfree.wind@vip.163.com>

From: gfree.wind@vip.163.com
Date: Mon,  4 Sep 2017 14:21:12 +0800

> From: Gao Feng <gfree.wind@vip.163.com>
> 
> The commit 520ac30f4551 ("net_sched: drop packets after root qdisc lock
> is released) made a big change of tc for performance. There are two points
> left in sch_prio and sch_qfq which are not changed with that commit. Now
> enhance them now with __qdisc_drop.
> 
> Signed-off-by: Gao Feng <gfree.wind@vip.163.com>

Thanks for catching these missed cases, applied.

^ permalink raw reply

* Re: [PATCH] ipv4: Namespaceify tcp_max_orphans knob
From: 严海双 @ 2017-09-09  5:09 UTC (permalink / raw)
  To: Cong Wang
  Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
	Eric Dumazet, Linux Kernel Network Developers, LKML
In-Reply-To: <CAM_iQpVDM0HwZd6c+Loych=nQE__eGteeJQ=XTOfHKgVxMRfrA@mail.gmail.com>



> On 2017年9月9日, at 下午12:35, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> 
> On Fri, Sep 8, 2017 at 6:25 PM, 严海双 <yanhaishuang@cmss.chinamobile.com> wrote:
>> 
>> 
>>> On 2017年9月9日, at 上午6:13, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> 
>>> On Wed, Sep 6, 2017 at 8:10 PM, Haishuang Yan
>>> <yanhaishuang@cmss.chinamobile.com> wrote:
>>>> Different namespace application might require different maximal number
>>>> of TCP sockets independently of the host.
>>> 
>>> So after your patch we could have N * net->ipv4.sysctl_tcp_max_orphans
>>> in a whole system, right? This just makes OOM easier to trigger.
>>> 
>> 
>> From my understanding, before the patch, we had N * net->ipv4.sysctl_tcp_max_orphans,
>> and after the patch, we could have ns1.sysctl_tcp_max_orphans + ns2.sysctl_tcp_max_orphans
>> + ns3.sysctl_tcp_max_orphans, is that right? Thanks for your reviewing.
> 
> Nope, by N I mean the number of containers. Before your patch, the limit
> is global, after your patch it is per container.
> 

Yeah, for example, if there is N containers, before the patch, I mean the limit is:

	N * net->ipv4.sysctl_tcp_max_orphans

After the patch, the limit is:

	ns1. net->ipv4.sysctl_tcp_max_orphans + ns2. net->ipv4.sysctl_tcp_max_orphans + …

^ permalink raw reply

* Re: [PATCH] ipv4: Namespaceify tcp_max_orphans knob
From: David Miller @ 2017-09-09  5:16 UTC (permalink / raw)
  To: yanhaishuang
  Cc: xiyou.wangcong, kuznet, yoshfuji, edumazet, netdev, linux-kernel
In-Reply-To: <E15E5C56-D712-4FEF-9AC5-AE28857D8D27@cmss.chinamobile.com>

From: 严海双 <yanhaishuang@cmss.chinamobile.com>
Date: Sat, 9 Sep 2017 13:09:57 +0800

> 
> 
>> On 2017年9月9日, at 下午12:35, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> 
>> On Fri, Sep 8, 2017 at 6:25 PM, 严海双 <yanhaishuang@cmss.chinamobile.com> wrote:
>>> 
>>> 
>>>> On 2017年9月9日, at 上午6:13, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>> 
>>>> On Wed, Sep 6, 2017 at 8:10 PM, Haishuang Yan
>>>> <yanhaishuang@cmss.chinamobile.com> wrote:
>>>>> Different namespace application might require different maximal number
>>>>> of TCP sockets independently of the host.
>>>> 
>>>> So after your patch we could have N * net->ipv4.sysctl_tcp_max_orphans
>>>> in a whole system, right? This just makes OOM easier to trigger.
>>>> 
>>> 
>>> From my understanding, before the patch, we had N * net->ipv4.sysctl_tcp_max_orphans,
>>> and after the patch, we could have ns1.sysctl_tcp_max_orphans + ns2.sysctl_tcp_max_orphans
>>> + ns3.sysctl_tcp_max_orphans, is that right? Thanks for your reviewing.
>> 
>> Nope, by N I mean the number of containers. Before your patch, the limit
>> is global, after your patch it is per container.
>> 
> 
> Yeah, for example, if there is N containers, before the patch, I mean the limit is:
> 
> 	N * net->ipv4.sysctl_tcp_max_orphans
> 
> After the patch, the limit is:
> 
> 	ns1. net->ipv4.sysctl_tcp_max_orphans + ns2. net->ipv4.sysctl_tcp_max_orphans + …

Not true.

Please remove "N" from your equation of the current situation.

"sysctl_tcp_max_orphans" applies to entire system, it is a global limit,
comparing one limit against all orphans in the system, there is no N.

^ permalink raw reply

* Re: [PATCH net-next v8] openvswitch: enable NSH support
From: kbuild test robot @ 2017-09-09  5:40 UTC (permalink / raw)
  To: Yi Yang
  Cc: kbuild-all, netdev, davem, dev, jbenc, e, blp, jan.scheurich,
	Yi Yang
In-Reply-To: <1504850299-84140-1-git-send-email-yi.y.yang@intel.com>

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

Hi Yi,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Yi-Yang/openvswitch-enable-NSH-support/20170909-124643
config: i386-randconfig-a1-201736 (attached as .config)
compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   net/openvswitch/actions.o: In function `do_execute_actions':
>> actions.c:(.text+0x19a7): undefined reference to `skb_push_nsh'
>> actions.c:(.text+0x19ee): undefined reference to `skb_pop_nsh'

---
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: 27829 bytes --]

^ permalink raw reply

* [PATCH v1 3/3] net: skb_queue_purge(): lock/unlock the queue only once
From: Michael Witten @ 2017-09-09  5:50 UTC (permalink / raw)
  To: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI
  Cc: Stephen Hemminger, Eric Dumazet, netdev, linux-kernel
In-Reply-To: <60c8906b751d4915be456009c220516e-mfwitten@gmail.com>

Thanks for your input, Eric Dumazet and Stephen Hemminger; based on
your observations, this version of the patch implements a very
lightweight purging of the queue.

To apply this patch, save this email to:

  /path/to/email

and then run:

  git am --scissors /path/to/email

You may also fetch this patch from GitHub:

  git checkout -b test 5969d1bb3082b41eba8fd2c826559abe38ccb6df
  git pull https://github.com/mfwitten/linux.git net/tcp-ip/01-cleanup/02

Sincerely,
Michael Witten

8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----

Hitherto, the queue's lock has been locked/unlocked every time
an item is dequeued; this seems not only inefficient, but also
incorrect, as the whole point of `skb_queue_purge()' is to clear
the queue, presumably without giving any other thread a chance to
manipulate the queue in the interim.

With this commit, the queue's lock is locked/unlocked only once
when `skb_queue_purge()' is called, and in a way that disables
the IRQs for only a minimal amount of time.

This is achieved by atomically re-initializing the queue (thereby
clearing it), and then freeing each of the items as though it were
enqueued in a private queue that doesn't require locking.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 net/core/skbuff.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 68065d7d383f..bd26b0bde784 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2825,18 +2825,28 @@ struct sk_buff *skb_dequeue_tail(struct sk_buff_head *list)
 EXPORT_SYMBOL(skb_dequeue_tail);
 
 /**
- *	skb_queue_purge - empty a list
- *	@list: list to empty
+ *	skb_queue_purge - empty a queue
+ *	@q: the queue to empty
  *
- *	Delete all buffers on an &sk_buff list. Each buffer is removed from
- *	the list and one reference dropped. This function takes the list
- *	lock and is atomic with respect to other list locking functions.
+ *	Dequeue and free each socket buffer that is in @q.
+ *
+ *	This function is atomic with respect to other queue-locking functions.
  */
-void skb_queue_purge(struct sk_buff_head *list)
+void skb_queue_purge(struct sk_buff_head *q)
 {
-	struct sk_buff *skb;
-	while ((skb = skb_dequeue(list)) != NULL)
+	unsigned long flags;
+	struct sk_buff *skb, *next, *head = (struct sk_buff *)q;
+
+	spin_lock_irqsave(&q->lock, flags);
+	skb = q->next;
+	__skb_queue_head_init(q);
+	spin_unlock_irqrestore(&q->lock, flags);
+
+	while (skb != head) {
+		next = skb->next;
 		kfree_skb(skb);
+		skb = next;
+	}
 }
 EXPORT_SYMBOL(skb_queue_purge);
 
-- 
2.14.1

^ permalink raw reply related

* Re: [PATCH net-next v8] openvswitch: enable NSH support
From: kbuild test robot @ 2017-09-09  7:10 UTC (permalink / raw)
  To: Yi Yang
  Cc: kbuild-all, netdev, davem, dev, jbenc, e, blp, jan.scheurich,
	Yi Yang
In-Reply-To: <1504850299-84140-1-git-send-email-yi.y.yang@intel.com>

[-- Attachment #1: Type: text/plain, Size: 677 bytes --]

Hi Yi,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Yi-Yang/openvswitch-enable-NSH-support/20170909-124643
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "skb_pop_nsh" [net/openvswitch/openvswitch.ko] undefined!
>> ERROR: "skb_push_nsh" [net/openvswitch/openvswitch.ko] undefined!

---
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: 39620 bytes --]

^ permalink raw reply

* [PATCH v2 net] net: qualcomm: rmnet: Fix a double free
From: Dan Carpenter @ 2017-09-09  8:58 UTC (permalink / raw)
  To: David S. Miller, Subash Abhinov Kasiviswanathan; +Cc: netdev, kernel-janitors
In-Reply-To: <2ddbb6da514108b4e70ccd8362292134@codeaurora.org>

There is a typo here so we accidentally free "skb" instead of "skbn".
It leads to a double free and a leak.  After discussing with Subash,
it's better to just move the check before the allocation and avoid the
need to free.

Fixes: ceed73a2cf4a ("drivers: net: ethernet: qualcomm: rmnet: Initial implementation")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Fix the leak as well.  Thanks Subash!

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 557c9bf1a469..86b8c758f94e 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -84,6 +84,10 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb)
 	if (((int)skb->len - (int)packet_len) < 0)
 		return NULL;
 
+	/* Some hardware can send us empty frames. Catch them */
+	if (ntohs(maph->pkt_len) == 0)
+		return NULL;
+
 	skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC);
 	if (!skbn)
 		return NULL;
@@ -94,11 +98,5 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb)
 	memcpy(skbn->data, skb->data, packet_len);
 	skb_pull(skb, packet_len);
 
-	/* Some hardware can send us empty frames. Catch them */
-	if (ntohs(maph->pkt_len) == 0) {
-		kfree_skb(skb);
-		return NULL;
-	}
-
 	return skbn;
 }

^ permalink raw reply related

* Re: Kernel 4.13.0-rc4-next-20170811 - IP Routing / Forwarding performance vs Core/RSS number / HT on
From: Paweł Staszewski @ 2017-09-09  9:03 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Paolo Abeni, Jesper Dangaard Brouer,
	Linux Kernel Network Developers, Alexander Duyck
In-Reply-To: <1502759849.4936.46.camel@edumazet-glaptop3.roam.corp.google.com>

Hi


Are there any plans to have this fix normally in kernel ?

Or it is mostly only hack - not longterm fix and need to be different ?


All tests that was done shows that without this patch there is about 
20-30% network forwarding performance degradation when using vlan 
interfaces


Thanks
Paweł



W dniu 2017-08-15 o 03:17, Eric Dumazet pisze:
> On Mon, 2017-08-14 at 18:07 -0700, Eric Dumazet wrote:
>
>> Or try to hack the IFF_XMIT_DST_RELEASE flag on the vlan netdev.
> Something like :
>
> diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
> index 5e831de3103e2f7092c7fa15534def403bc62fb4..9472de846d5c0960996261cb2843032847fa4bf7 100644
> --- a/net/8021q/vlan_netlink.c
> +++ b/net/8021q/vlan_netlink.c
> @@ -143,6 +143,7 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev,
>   	vlan->vlan_proto = proto;
>   	vlan->vlan_id	 = nla_get_u16(data[IFLA_VLAN_ID]);
>   	vlan->real_dev	 = real_dev;
> +	dev->priv_flags |= (real_dev->priv_flags & IFF_XMIT_DST_RELEASE);
>   	vlan->flags	 = VLAN_FLAG_REORDER_HDR;
>   
>   	err = vlan_check_real_dev(real_dev, vlan->vlan_proto, vlan->vlan_id);
>
>
>
>

^ permalink raw reply

* Re: [PATCH] ipv4: Namespaceify tcp_max_orphans knob
From: 严海双 @ 2017-09-09 10:21 UTC (permalink / raw)
  To: David Miller
  Cc: xiyou.wangcong, kuznet, yoshfuji, edumazet, netdev, linux-kernel
In-Reply-To: <20170908.221648.186026315535806669.davem@davemloft.net>



> On 2017年9月9日, at 下午1:16, David Miller <davem@davemloft.net> wrote:
> 
> From: 严海双 <yanhaishuang@cmss.chinamobile.com>
> Date: Sat, 9 Sep 2017 13:09:57 +0800
> 
>> 
>> 
>>> On 2017年9月9日, at 下午12:35, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> 
>>> On Fri, Sep 8, 2017 at 6:25 PM, 严海双 <yanhaishuang@cmss.chinamobile.com> wrote:
>>>> 
>>>> 
>>>>> On 2017年9月9日, at 上午6:13, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>>> 
>>>>> On Wed, Sep 6, 2017 at 8:10 PM, Haishuang Yan
>>>>> <yanhaishuang@cmss.chinamobile.com> wrote:
>>>>>> Different namespace application might require different maximal number
>>>>>> of TCP sockets independently of the host.
>>>>> 
>>>>> So after your patch we could have N * net->ipv4.sysctl_tcp_max_orphans
>>>>> in a whole system, right? This just makes OOM easier to trigger.
>>>>> 
>>>> 
>>>> From my understanding, before the patch, we had N * net->ipv4.sysctl_tcp_max_orphans,
>>>> and after the patch, we could have ns1.sysctl_tcp_max_orphans + ns2.sysctl_tcp_max_orphans
>>>> + ns3.sysctl_tcp_max_orphans, is that right? Thanks for your reviewing.
>>> 
>>> Nope, by N I mean the number of containers. Before your patch, the limit
>>> is global, after your patch it is per container.
>>> 
>> 
>> Yeah, for example, if there is N containers, before the patch, I mean the limit is:
>> 
>> 	N * net->ipv4.sysctl_tcp_max_orphans
>> 
>> After the patch, the limit is:
>> 
>> 	ns1. net->ipv4.sysctl_tcp_max_orphans + ns2. net->ipv4.sysctl_tcp_max_orphans + …
> 
> Not true.
> 
> Please remove "N" from your equation of the current situation.
> 
> "sysctl_tcp_max_orphans" applies to entire system, it is a global limit,
> comparing one limit against all orphans in the system, there is no N.

Yes, it’s right. I browse the source code and found that it’s a global limit, 
sorry for my mistake.

Thanks David and Cong.

^ permalink raw reply

* Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
From: Nikolay Aleksandrov @ 2017-09-09 10:29 UTC (permalink / raw)
  To: Mahesh Bandewar (महेश बंडेवार)
  Cc: Kosuke Tatsukawa, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Reinis Rozitis
In-Reply-To: <CAF2d9jgHLJ28hpiSafL7823-Ga4y8aYZ2t_Sqh7D51VL_DgDww@mail.gmail.com>

On 09/09/17 02:54, Mahesh Bandewar (महेश बंडेवार) wrote:
> On Fri, Sep 8, 2017 at 7:30 AM, Nikolay Aleksandrov
> <nikolay@cumulusnetworks.com> wrote:
>> On 08/09/17 17:17, Kosuke Tatsukawa wrote:
>>> Hi,
>>>
>>>> On 08/09/17 13:10, Nikolay Aleksandrov wrote:
>>>>> On 08/09/17 05:06, Kosuke Tatsukawa wrote:
>>>>>> Hi,
>>>>>>
>>>>>>> On  7.09.2017 01:47, Kosuke Tatsukawa wrote:
>>>>>>>> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
>>>>>>>> balance-alb mode") tried to fix transmit dynamic load balancing in
>>>>>>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4
>>>>>>>> ("bonding: remove hardcoded value").
>>>>>>>>
>>>>>>>> It turned out that my previous patch only fixed the case when
>>>>>>>> balance-alb was specified as bonding module parameter, and not when
>>>>>>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
>>>>>>>> common usage).  In the latter case, tlb_dynamic_lb was set up according
>>>>>>>> to the default mode of the bonding interface, which happens to be
>>>>>>>> balance-rr.
>>>>>>>>
>>>>>>>> This additional patch addresses this issue by setting up tlb_dynamic_lb
>>>>>>>> to 1 if "mode" is set to balance-alb through the sysfs interface.
>>>>>>>>
>>>>>>>> I didn't add code to change tlb_balance_lb back to the default value for
>>>>>>>> other modes, because "mode" is usually set up only once during
>>>>>>>> initialization, and it's not worthwhile to change the static variable
>>>>>>>> bonding_defaults in bond_main.c to a global variable just for this
>>>>>>>> purpose.
>>>>>>>>
>>>>>>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
>>>>>>>> balance-tlb mode if it is set up using the sysfs interface.  I didn't
>>>>>>>> change that behavior, because the value of tlb_balance_lb can be changed
>>>>>>>> using the sysfs interface for balance-tlb, and I didn't like changing
>>>>>>>> the default value back and forth for balance-tlb.
>>>>>>>>
>>>>>>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
>>>>>>>> written to.  However, I think balance-alb with tlb_dynamic_lb set to 0
>>>>>>>> is not an intended usage, so there is little use making it writable at
>>>>>>>> this moment.
>>>>>>>>
>>>>>>>> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
>>>>>>>> Reported-by: Reinis Rozitis <r@roze.lv>
>>>>>>>> Signed-off-by: Kosuke Tatsukawa <tatsu@ab.jp.nec.com>
>>>>>>>> Cc: stable@vger.kernel.org  # v4.12+
>>>>>>>> ---
>>>>>>>>  drivers/net/bonding/bond_options.c |    3 +++
>>>>>>>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>>>>
>>>>>>>
>>>>>>> I don't believe this to be the right solution, hardcoding it like this
>>>>>>> changes user-visible behaviour. The issue is that if someone configured
>>>>>>> it to be 0 in tlb mode, suddenly it will become 1 and will silently
>>>>>>> override their config if they switch the mode to alb. Also it robs users
>>>>>>> from their choice.
>>>>>>>
>>>>>>> If you think this should be settable in ALB mode, then IMO you should
>>>>>>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
>>>>>>> That would also be consistent with how it's handled in TLB mode.
>>>>>>
>>>>>> No, I don't think tlb_dynamic_lb should be settable in balance-alb at
>>>>>> this point.  All the current commits regarding tlb_dynamic_lb are for
>>>>>> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set
>>>>>> to 0 is an intended usage.
>>>>>>
>>>>>>
>>>>>>> Going back and looking at your previous fix I'd argue that it is also
>>>>>>> wrong, you should've removed the mode check altogether to return the
>>>>>>> original behaviour where the dynamic_lb is set to 1 (enabled) by
>>>>>>> default and then ALB mode would've had it, of course that would've left
>>>>>>> the case of setting it to 0 in TLB mode and switching to ALB, but that
>>>>>>> is a different issue.
>>>>>>
>>>>>> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.
>>>>>> tlb_dynamic_lb is referenced in the following functions.
>>>>>>
>>>>>>  + bond_do_alb_xmit()  -- Used by both balance-tlb and balance-alb
>>>>>>  + bond_tlb_xmit()  -- Only used by balance-tlb
>>>>>>  + bond_open()  -- Used by both balance-tlb and balance-alb
>>>>>>  + bond_check_params()  -- Used during module initialization
>>>>>>  + bond_fill_info()  -- Used to get/set value
>>>>>>  + bond_option_tlb_dynamic_lb_set()  -- Used to get/set value
>>>>>>  + bonding_show_tlb_dynamic_lb()  -- Used to get/set value
>>>>>>  + bond_is_nondyn_tlb()  -- Only referenced if balance-tlb mode
>>>>>>
>>>>>> The following untested patch adds code to make balance-alb work as if
>>>>>> tlb_dynamic_lb==1 for the functions which affect balance-alb mode.  It
>>>>>> also reverts my previous patch.
>>>>>>
>>>>>> What do you think about this approach?
>>>>>> ---
>>>>>> Kosuke TATSUKAWA  | 1st Platform Software Division
>>>>>>                   | NEC Solution Innovators
>>>>>>                   | tatsu@ab.jp.nec.com
>>>>>>
>>>>>
>>>>> Logically the approach looks good, that being said it adds unnecessary tests in
>>>>> the fast path, why not just something like the patch below ? That only leaves the
>>>>> problem if it is zeroed in TLB and switched to ALB mode, and that is a one line
>>>>> fix to unsuppmodes just allow it to be set for that specific case. The below
>>>>> returns the default behaviour before the commit in your Fixes tag.
>>>>>
>>>>>
>>>>
>>>> Actually I'm fine with your approach, too. It will fix this regardless of the
>>>> value of tlb_dynamic_lb which sounds good to me for the price of a test in
>>>> the fast path.
>>>
>>> If you're concerned about the additional test in the fast path, how
>>> about the patch below.  I've added an arguemnt to bond_do_alb_xmit()
>>> to handle both balance-tlb and balance-alb similary.
>>>
>>
>> Even better, looks great! 1 question below though.
>>
>>> I'm not sure if this causes any problem if tlb_dynamic_lb is changed
>>> while calling bond_do_alb_xmit() in balance-tlb mode.
>>
>> The option has the ifdown flag, you shouldn't be able to change it while
>> the bond dev is up, but even if you could I don't think it will be an issue
>> for the xmit.
>>
>>> ---
>>> Kosuke TATSUKAWA  | 1st Platform Software Division
>>>                   | NEC Solution Innovators
>>>                   | tatsu@ab.jp.nec.com
>>>
>>> ------------------------------------------------------------------------
>>>  drivers/net/bonding/bond_alb.c  |   11 ++++++-----
>>>  drivers/net/bonding/bond_main.c |    5 +++--
>>>  2 files changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>>> index c02cc81..7710f20 100644
>>> --- a/drivers/net/bonding/bond_alb.c
>>> +++ b/drivers/net/bonding/bond_alb.c
>>> @@ -1317,7 +1317,7 @@ void bond_alb_deinitialize(struct bonding *bond)
>>>  }
>>>
>>>  static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>>> -                         struct slave *tx_slave)
>>> +                         struct slave *tx_slave, int tlb_dynamic_lb)
>>>  {
>>>       struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>>>       struct ethhdr *eth_data = eth_hdr(skb);
>>> @@ -1325,7 +1325,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>>>       if (!tx_slave) {
>>>               /* unbalanced or unassigned, send through primary */
>>>               tx_slave = rcu_dereference(bond->curr_active_slave);
>>> -             if (bond->params.tlb_dynamic_lb)
>>> +             if (tlb_dynamic_lb)
>>>                       bond_info->unbalanced_load += skb->len;
>>>       }
>>>
>>> @@ -1339,7 +1339,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>>>               goto out;
>>>       }
>>>
>>> -     if (tx_slave && bond->params.tlb_dynamic_lb) {
>>> +     if (tx_slave && tlb_dynamic_lb) {
>>>               spin_lock(&bond->mode_lock);
>>>               __tlb_clear_slave(bond, tx_slave, 0);
>>>               spin_unlock(&bond->mode_lock);
>>> @@ -1386,7 +1386,8 @@ int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>>                       break;
>>>               }
>>>       }
>>> -     return bond_do_alb_xmit(skb, bond, tx_slave);
>>> +     return bond_do_alb_xmit(skb, bond, tx_slave,
>>> +                             bond->params.tlb_dynamic_lb);
>>>  }
>>>
>>>  int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>> @@ -1483,7 +1484,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>>               tx_slave = tlb_choose_channel(bond, hash_index, skb->len);
>>>       }
>>>
>>> -     return bond_do_alb_xmit(skb, bond, tx_slave);
>>> +     return bond_do_alb_xmit(skb, bond, tx_slave, 1);
>>>  }
>>>
>>>  void bond_alb_monitor(struct work_struct *work)
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index fc63992..bcb71e7 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -3305,7 +3305,8 @@ static int bond_open(struct net_device *bond_dev)
>>>                */
>>>               if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))
>>>                       return -ENOMEM;
>>> -             if (bond->params.tlb_dynamic_lb)
>>> +             if (bond->params.tlb_dynamic_lb ||
>>> +                 (BOND_MODE(bond) == BOND_MODE_TLB))
>>
>> mode == tlb ? shouldn't this check be for alb ?
>>
> Actually this is not needed since this is already inside if
> (bond_is_lb()) condition.
> 
>>>                       queue_delayed_work(bond->wq, &bond->alb_work, 0);
>>>       }
>>>
>>> @@ -4601,7 +4602,7 @@ static int bond_check_params(struct bond_params *params)
>>>       }
>>>       ad_user_port_key = valptr->value;
>>>
>>> -     if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {
>>> +     if (bond_mode == BOND_MODE_TLB) {
>>>               bond_opt_initstr(&newval, "default");
>>>               valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
>>>                                       &newval);
>>>
>>
> I think the underlying issue is that tlb_dynamic_lb should be set to 1
> for all modes which was not the case when it was getting initialized
> only forTLB mode. So from that perspective I prefer Nik's patch with a
> small variation that guards the case when mode transitions from TLB to
> ALB. The reason why I like that patch is because it's simple and
> avoids complications.

+1, I think this is the most straight-forward solution as well and
safest for -net

I will go ahead and submit it in a few minutes.

Thanks,
 Nik

> 
> Here is what I meant -
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index fc63992ab0e0..c99dc59d729b 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4289,7 +4289,7 @@ static int bond_check_params(struct bond_params *params)
>         int bond_mode   = BOND_MODE_ROUNDROBIN;
>         int xmit_hashtype = BOND_XMIT_POLICY_LAYER2;
>         int lacp_fast = 0;
> -       int tlb_dynamic_lb = 0;
> +       int tlb_dynamic_lb;
> 
>         /* Convert string parameters. */
>         if (mode) {
> @@ -4601,16 +4601,13 @@ static int bond_check_params(struct bond_params *params)
>         }
>         ad_user_port_key = valptr->value;
> 
> -       if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {
> -               bond_opt_initstr(&newval, "default");
> -               valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
> -                                       &newval);
> -               if (!valptr) {
> -                       pr_err("Error: No tlb_dynamic_lb default value");
> -                       return -EINVAL;
> -               }
> -               tlb_dynamic_lb = valptr->value;
> +       bond_opt_initstr(&newval, "default");
> +       valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB), &newval);
> +       if (!valptr) {
> +               pr_err("Error: No tlb_dynamic_lb default value");
> +               return -EINVAL;
>         }
> +       tlb_dynamic_lb = valptr->value;
> 
>         if (lp_interval == 0) {
>                 pr_warn("Warning: ip_interval must be between 1 and
> %d, so it was reset to %d\n",
> diff --git a/drivers/net/bonding/bond_options.c
> b/drivers/net/bonding/bond_options.c
> index a12d603d41c6..7feacd262182 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -754,6 +754,12 @@ static int bond_option_mode_set(struct bonding *bond,
>                            bond->params.miimon);
>         }
> 
> +       /* Guard against transition TLB/tlb_dynamic_lb=0 -> ALB mode.
> +        * In ALB mode, tlb_dynamic_lb must be set to 1.
> +        */
> +       if (newval->value == BOND_MODE_ALB && bond->params.tlb_dynamic_lb != 1)
> +               bond->params.tlb_dynamic_lb = 1;
> +
>         /* don't cache arp_validate between modes */
>         bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
>         bond->params.mode = newval->value;
> 

^ permalink raw reply

* Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
From: Nikolay Aleksandrov @ 2017-09-09 11:23 UTC (permalink / raw)
  To: Mahesh Bandewar (महेश बंडेवार)
  Cc: Kosuke Tatsukawa, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Reinis Rozitis
In-Reply-To: <9ae82441-78da-0056-88c1-2f9d863fb81f@cumulusnetworks.com>

On 09/09/17 13:29, Nikolay Aleksandrov wrote:
> On 09/09/17 02:54, Mahesh Bandewar (महेश बंडेवार) wrote:
>> On Fri, Sep 8, 2017 at 7:30 AM, Nikolay Aleksandrov
>> <nikolay@cumulusnetworks.com> wrote:
>>> On 08/09/17 17:17, Kosuke Tatsukawa wrote:
[snip]
>>>
>> I think the underlying issue is that tlb_dynamic_lb should be set to 1
>> for all modes which was not the case when it was getting initialized
>> only forTLB mode. So from that perspective I prefer Nik's patch with a
>> small variation that guards the case when mode transitions from TLB to
>> ALB. The reason why I like that patch is because it's simple and
>> avoids complications.
> 
> +1, I think this is the most straight-forward solution as well and
> safest for -net
> 
> I will go ahead and submit it in a few minutes.
> 
> Thanks,
>  Nik
> 

Just FYI, since the second fix (tlb_dynamic_lb in TLB = 0 switch to ALB) is identical
to this patch, I'm acking this one and will wait until it's in to submit the default
value fix (if we start in non-TLB mode and switch to TLB we'll get dynamic_lb = 0
currently).

I guess this is the simplest way, I didn't want to alter user-configured value on
mode switch, but it seems better than the alternative especially for -net.

^ permalink raw reply

* Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
From: Nikolay Aleksandrov @ 2017-09-09 11:28 UTC (permalink / raw)
  To: Kosuke Tatsukawa, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek
  Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Reinis Rozitis
In-Reply-To: <17EC94B0A072C34B8DCF0D30AD16044A02985CB5@BPXM09GP.gisp.nec.co.jp>

On 07/09/17 01:47, Kosuke Tatsukawa wrote:
> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
> balance-alb mode") tried to fix transmit dynamic load balancing in
> balance-alb mode, which wasn't working after commit 8b426dc54cf4
> ("bonding: remove hardcoded value").
> 
> It turned out that my previous patch only fixed the case when
> balance-alb was specified as bonding module parameter, and not when
> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
> common usage).  In the latter case, tlb_dynamic_lb was set up according
> to the default mode of the bonding interface, which happens to be
> balance-rr.
> 
> This additional patch addresses this issue by setting up tlb_dynamic_lb
> to 1 if "mode" is set to balance-alb through the sysfs interface.
> 
> I didn't add code to change tlb_balance_lb back to the default value for
> other modes, because "mode" is usually set up only once during
> initialization, and it's not worthwhile to change the static variable
> bonding_defaults in bond_main.c to a global variable just for this
> purpose.
> 
> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
> balance-tlb mode if it is set up using the sysfs interface.  I didn't
> change that behavior, because the value of tlb_balance_lb can be changed
> using the sysfs interface for balance-tlb, and I didn't like changing
> the default value back and forth for balance-tlb.
> 
> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
> written to.  However, I think balance-alb with tlb_dynamic_lb set to 0
> is not an intended usage, so there is little use making it writable at
> this moment.
> 
> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
> Reported-by: Reinis Rozitis <r@roze.lv>
> Signed-off-by: Kosuke Tatsukawa <tatsu@ab.jp.nec.com>
> Cc: stable@vger.kernel.org  # v4.12+
> ---
>  drivers/net/bonding/bond_options.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 

This fix is simpler and more suitable for -net, it fixes the case where
we switch to ALB mode with tlb_dynamic_lb = 0. After it is in I'll fix the
default tlb_dynamic_lb issue and restore the original behaviour.

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

^ permalink raw reply

* Re: [PATCH RFC 3/5] Add KSZ8795 switch driver
From: Andrew Lunn @ 2017-09-09 15:45 UTC (permalink / raw)
  To: Tristram.Ha
  Cc: pavel, muvarov, nathan.leigh.conrad, vivien.didelot, f.fainelli,
	netdev, linux-kernel, Woojung.Huh
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41123010@CHN-SV-EXMX02.mchp-main.com>

On Sat, Sep 09, 2017 at 01:44:37AM +0000, Tristram.Ha@microchip.com wrote:
> > -----Original Message-----
> > From: Pavel Machek [mailto:pavel@ucw.cz]
> > Sent: Friday, September 08, 2017 2:58 PM
> > To: Tristram Ha - C24268
> > Cc: andrew@lunn.ch; muvarov@gmail.com; nathan.leigh.conrad@gmail.com;
> > vivien.didelot@savoirfairelinux.com; f.fainelli@gmail.com;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Woojung Huh -
> > C21699
> > Subject: Re: [PATCH RFC 3/5] Add KSZ8795 switch driver
> > 
> > Hi!
> > 
> > > > > +	default:
> > > > > +		processed = false;
> > > > > +		break;
> > > > > +	}
> > > > > +	if (processed)
> > > > > +		*val = data;
> > > > > +}
> > > >
> > > > Similar code will be needed by other drivers, right?
> > >
> > > Although KSZ8795 and KSZ8895 may use the same code, the other
> > > chips will have different code.
> > 
> > Ok, please make sure code is shared between these two.
> 
> The exact function probably cannot be shared between KSZ8795
> and KSZ8895 drivers because although the register constants look
> the same but their exact locations are different in the 2 chips.

You need to be careful with such functions. If they look identical,
somebody will try to consolidate them into one. So either you need to
do the consolidation yourself in order to get it right, or you need to
add a comment about how it differs.

    Andrew

^ permalink raw reply

* Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions
From: Andrew Lunn @ 2017-09-09 15:55 UTC (permalink / raw)
  To: Jassi Brar
  Cc: Florian Fainelli, Kunihiko Hayashi, netdev, David S. Miller,
	Rob Herring, Mark Rutland, linux-arm-kernel@lists.infradead.org,
	lkml, Devicetree List, Masahiro Yamada, Masami Hiramatsu,
	Jongsung Kim
In-Reply-To: <CAJe_ZhciWBhw5npxEjTUGw2duS7DSfhzP5nzAOy_ig4-1XcPWw@mail.gmail.com>

On Sat, Sep 09, 2017 at 09:03:05AM +0530, Jassi Brar wrote:
> On 9 September 2017 at 00:21, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> >> From: Jassi Brar <jaswinder.singh@linaro.org>
> >>
> >> Add RTL8201F phy-id and the related functions to the driver.
> >>
> >> The original patch is as follows:
> >> https://patchwork.kernel.org/patch/2538341/
> >>
> >> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> >> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> >> ---
> >>  drivers/net/phy/realtek.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 45 insertions(+)
> >>
> >> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> >> index 9cbe645..d9974ce 100644
> >> --- a/drivers/net/phy/realtek.c
> >> +++ b/drivers/net/phy/realtek.c
> >> @@ -29,10 +29,23 @@
> >>  #define RTL8211F_PAGE_SELECT 0x1f
> >>  #define RTL8211F_TX_DELAY    0x100
> >>
> >> +#define RTL8201F_ISR         0x1e
> >> +#define RTL8201F_PAGE_SELECT 0x1f
> >
> > We have a page select register define for the RTL8211F right above, so
> > surely we can make that a common definition?
> >
> That is just for the sake of consistency.
> I mean RTL8211 wouldn't look neat among everything else RTL8201.
> 
> Also the page-select offsets just _happen_ to be same value...

If you look at all the other supported PHYs, they all consistently use
the same page register across models. Marvell is always 22, mscc is
always 31, vitesse is always 31.

I would say it is a safe bet that all realtek PHYs will use 0x1f for
page select. So please add a patch which renames RTL8211F_PAGE_SELECT
to RTL821x_PAGE_SELECT.

It is best to do this now. I spent a while cleaning up the mess the
Marvell driver had got into with its page select code. Lots of
duplicate code and defines doing the same thing.

	  Andrew

^ permalink raw reply

* Re: [PATCH iproute2 3/3] bridge: request vlans along with link information
From: Roopa Prabhu @ 2017-09-09 16:24 UTC (permalink / raw)
  To: Roman Mashak
  Cc: stephen@networkplumber.org, netdev@vger.kernel.org,
	Jamal Hadi Salim
In-Reply-To: <1504907543-14394-4-git-send-email-mrv@mojatatu.com>

On Fri, Sep 8, 2017 at 2:52 PM, Roman Mashak <mrv@mojatatu.com> wrote:
> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
> ---
>  bridge/link.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/bridge/link.c b/bridge/link.c
> index 60200f1..9e4206f 100644
> --- a/bridge/link.c
> +++ b/bridge/link.c
> @@ -461,9 +461,19 @@ static int brlink_show(int argc, char **argv)
>                 }
>         }
>
> -       if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
> -               perror("Cannon send dump request");
> -               exit(1);
> +       if (show_details) {
> +               if (rtnl_wilddump_req_filter(&rth, PF_BRIDGE, RTM_GETLINK,
> +                                            (compress_vlans ?
> +                                             RTEXT_FILTER_BRVLAN_COMPRESSED :
> +                                             RTEXT_FILTER_BRVLAN)) < 0) {
> +                       perror("Cannon send dump request");
> +                       exit(1);
> +               }

vlan information is already available with `bridge vlan show`. any
specific reason why you want it in
the link dump output ?

The problem is this might just make the link dump larger and also add
too much clutter into the regular link dump output. iproute2 detailed
dump is already a bit hard to interpret. And without compression by
default, vlan info can just take over the link dump output. It will be
hard to look for other link attributes after that :). We deploy with
thousands of vlans and without compression even bridge vlan default
output is already hard to interpret.

Can you please paste a sample default detailed output with your patch
with a few hundred vlans ?

^ permalink raw reply

* Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver
From: Florian Fainelli @ 2017-09-09 16:30 UTC (permalink / raw)
  To: Kunihiko Hayashi, netdev, David S. Miller, Andrew Lunn
  Cc: Rob Herring, Mark Rutland, linux-arm-kernel, linux-kernel,
	devicetree, Masahiro Yamada, Masami Hiramatsu, Jassi Brar
In-Reply-To: <1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>



On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> The UniPhier platform from Socionext provides the AVE ethernet
> controller that includes MAC and MDIO bus supporting RGMII/RMII
> modes. The controller is named AVE.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---

[snip]

> +static int ave_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct ave_private *priv = netdev_priv(ndev);
> +	u32 proc_idx, done_idx, ndesc, cmdsts;
> +	int freepkt;
> +	unsigned char *buffptr = NULL; /* buffptr for descriptor */
> +	unsigned int len;
> +	dma_addr_t paddr;
> +
> +	proc_idx = priv->tx.proc_idx;
> +	done_idx = priv->tx.done_idx;
> +	ndesc = priv->tx.ndesc;
> +	freepkt = ((done_idx + ndesc - 1) - proc_idx) % ndesc;
> +
> +	/* not enough entry, then we stop queue */
> +	if (unlikely(freepkt < 2)) {
> +		netif_stop_queue(ndev);
> +		if (unlikely(freepkt < 1))
> +			return NETDEV_TX_BUSY;

This looks wrong, why are you checking first for less than 2
descriptors, and if there is none, NETDEV_TX_BUSY? If you need 2 slots
to complete a transmision, stop the transmit queue and return
NETDEV_TX_BUSY.

> +	}
> +
> +	priv->tx.desc[proc_idx].skbs = skb;
> +
> +	/* add padding for short packet */
> +	if (skb_padto(skb, ETH_ZLEN)) {
> +		dev_kfree_skb_any(skb);
> +		return NETDEV_TX_OK;
> +	}

skb_padto() frees the SKB in case of error, that would lead to a double
free here.

> +
> +	buffptr = skb->data - NET_IP_ALIGN;
> +	len = max_t(unsigned int, ETH_ZLEN, skb->len);

If you use skb_put_padto() if padding was necessary skb->len will be at
least ETH_ZLEN, so you can remove this.

> +
> +	paddr = ave_dma_map(ndev, &priv->tx.desc[proc_idx], buffptr,
> +			    len + NET_IP_ALIGN, DMA_TO_DEVICE);

As mentioned before you can't assume this will never fail.

> +	paddr += NET_IP_ALIGN;
> +
> +	/* set buffer address to descriptor */
> +	ave_wdesc_addr(ndev, AVE_DESCID_TX, proc_idx, 4, paddr);

Also mentioned in the other email, make this 4 a constant so we know
it's an offset and not a length.

> +
> +	/* set flag and length to send */
> +	cmdsts = AVE_STS_OWN | AVE_STS_1ST | AVE_STS_LAST
> +		| (len & AVE_STS_PKTLEN_TX);

AVE_STS_PKTLEN_TX would be better named with a _MASK suffix.

> +
> +	/* set interrupt per AVE_FORCE_TXINTCNT or when queue is stopped */
> +	if (!(proc_idx % AVE_FORCE_TXINTCNT) || netif_queue_stopped(ndev))
> +		cmdsts |= AVE_STS_INTR;
> +
> +	/* disable checksum calculation when skb doesn't calurate checksum */
> +	if (skb->ip_summed == CHECKSUM_NONE ||
> +	    skb->ip_summed == CHECKSUM_UNNECESSARY)
> +		cmdsts |= AVE_STS_NOCSUM;
> +
> +	/* set cmdsts */
> +	ave_wdesc(ndev, AVE_DESCID_TX, proc_idx, 0, cmdsts);
> +
> +	priv->tx.proc_idx = (proc_idx + 1) % ndesc;

You should also check the ring space after transmission and assert flow
control on the transmit queue if needed.

> +
> +	return NETDEV_TX_OK;
> +}

[snip]

> +static struct net_device_stats *ave_stats(struct net_device *ndev)
> +{
> +	struct ave_private *priv = netdev_priv(ndev);
> +	u32 drop_num = 0;
> +
> +	priv->stats.rx_errors = ave_r32(ndev, AVE_BFCR);
> +
> +	drop_num += ave_r32(ndev, AVE_RX0OVFFC);
> +	drop_num += ave_r32(ndev, AVE_SN5FC);
> +	drop_num += ave_r32(ndev, AVE_SN6FC);
> +	drop_num += ave_r32(ndev, AVE_SN7FC);
> +	priv->stats.rx_dropped = drop_num;
> +

You should consider switching to 64-bit statistics, this requires a
little bit more work for 32-bit hosts (see
include/linux/u64_stats_sync.h) but this allows you to keep statistics
around above 4GB.

> +	return &priv->stats;
> +}
> +-- 
Florian

^ permalink raw reply

* Re: [PATCH v1 3/3] net: skb_queue_purge(): lock/unlock the queue only once
From: Eric Dumazet @ 2017-09-09 16:52 UTC (permalink / raw)
  To: Michael Witten
  Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
	Stephen Hemminger, netdev, linux-kernel
In-Reply-To: <0c67e720495b4c1a920ed0f7a7e1daeb-mfwitten@gmail.com>

On Sat, 2017-09-09 at 05:50 +0000, Michael Witten wrote:
> Thanks for your input, Eric Dumazet and Stephen Hemminger; based on
> your observations, this version of the patch implements a very
> lightweight purging of the queue.

net-next is closed.

Documentation/networking/netdev-FAQ.txt

Meaning we are chasing bugs at this moment, not adding new ones.

Thanks.

^ permalink raw reply

* Re: [PATCH iproute2 3/3] bridge: request vlans along with link information
From: Jamal Hadi Salim @ 2017-09-09 17:23 UTC (permalink / raw)
  To: Roopa Prabhu, Roman Mashak
  Cc: stephen@networkplumber.org, netdev@vger.kernel.org
In-Reply-To: <CAJieiUhT75fM3LuhNW32QkbJA9TO5dHwb_G-aRD2H70jF-FG2A@mail.gmail.com>

On 17-09-09 12:24 PM, Roopa Prabhu wrote:
> On Fri, Sep 8, 2017 at 2:52 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>> ---
>>   bridge/link.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/bridge/link.c b/bridge/link.c
>> index 60200f1..9e4206f 100644
>> --- a/bridge/link.c
>> +++ b/bridge/link.c
>> @@ -461,9 +461,19 @@ static int brlink_show(int argc, char **argv)
>>                  }
>>          }
>>
>> -       if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
>> -               perror("Cannon send dump request");
>> -               exit(1);
>> +       if (show_details) {
>> +               if (rtnl_wilddump_req_filter(&rth, PF_BRIDGE, RTM_GETLINK,
>> +                                            (compress_vlans ?
>> +                                             RTEXT_FILTER_BRVLAN_COMPRESSED :
>> +                                             RTEXT_FILTER_BRVLAN)) < 0) {
>> +                       perror("Cannon send dump request");
>> +                       exit(1);
>> +               }
> 
> vlan information is already available with `bridge vlan show`. any
> specific reason why you want it in
> the link dump output ?
> 
 >
> The problem is this might just make the link dump larger and also add
> too much clutter into the regular link dump output. iproute2 detailed
> dump is already a bit hard to interpret. And without compression by
> default, vlan info can just take over the link dump output. It will be
> hard to look for other link attributes after that :). We deploy with
> thousands of vlans and without compression even bridge vlan default
> output is already hard to interpret.
>

Agree we should be turning on this stuff by default. i.e default stays
compressed; otherwise it a huge dump.

Having said that there is a lot of mess with this stuff.
The bridge link events _already send this IFLA_AF_SPCE info_
so not much choice  there but to print it.
At minimal we need that part because unfortunately there is no
vlanfilter event in existence which will send us summaries of just
vlans added to a port i.e both use XXXLINK.
In general, the XXXLINK interface with these master devices (bridge,
bond etc) continues to get messy. Recently started seeing events with
devices claiming to be of KIND_slave etc on the bridge and bond devices;
yet at the same time my wireless card events are also showing up on the
bridge link even though it is not enslaved there..

cheers,
jamal

> Can you please paste a sample default detailed output with your patch
> with a few hundred vlans ?
> 

^ permalink raw reply

* Re: [PATCH iproute2 3/3] bridge: request vlans along with link information
From: Nikolay Aleksandrov @ 2017-09-09 18:15 UTC (permalink / raw)
  To: Jamal Hadi Salim, Roopa Prabhu, Roman Mashak
  Cc: stephen@networkplumber.org, netdev@vger.kernel.org
In-Reply-To: <5706e1ba-6b85-4f32-a858-b5d87b17eb51@mojatatu.com>

On 09/09/17 20:23, Jamal Hadi Salim wrote:
> On 17-09-09 12:24 PM, Roopa Prabhu wrote:
>> On Fri, Sep 8, 2017 at 2:52 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>>> ---
>>>   bridge/link.c | 16 +++++++++++++---
>>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/bridge/link.c b/bridge/link.c
>>> index 60200f1..9e4206f 100644
>>> --- a/bridge/link.c
>>> +++ b/bridge/link.c
>>> @@ -461,9 +461,19 @@ static int brlink_show(int argc, char **argv)
>>>                  }
>>>          }
>>>
>>> -       if (rtnl_wilddump_request(&rth, PF_BRIDGE, RTM_GETLINK) < 0) {
>>> -               perror("Cannon send dump request");
>>> -               exit(1);
>>> +       if (show_details) {
>>> +               if (rtnl_wilddump_req_filter(&rth, PF_BRIDGE, RTM_GETLINK,
>>> +                                            (compress_vlans ?
>>> +                                             RTEXT_FILTER_BRVLAN_COMPRESSED :
>>> +                                             RTEXT_FILTER_BRVLAN)) < 0) {
>>> +                       perror("Cannon send dump request");
>>> +                       exit(1);
>>> +               }
>>
>> vlan information is already available with `bridge vlan show`. any
>> specific reason why you want it in
>> the link dump output ?
>>
>>
>> The problem is this might just make the link dump larger and also add
>> too much clutter into the regular link dump output. iproute2 detailed
>> dump is already a bit hard to interpret. And without compression by
>> default, vlan info can just take over the link dump output. It will be
>> hard to look for other link attributes after that :). We deploy with
>> thousands of vlans and without compression even bridge vlan default
>> output is already hard to interpret.
>>
> 
> Agree we should be turning on this stuff by default. i.e default stays
> compressed; otherwise it a huge dump.

I think this should be dumped with the getlink request only on some additional
flag. The getlink does not include these by default.

> 
> Having said that there is a lot of mess with this stuff.
> The bridge link events _already send this IFLA_AF_SPCE info_
> so not much choice  there but to print it.

Right, on NEWLINK per port notification you'll get the compressed vlan info.

> At minimal we need that part because unfortunately there is no
> vlanfilter event in existence which will send us summaries of just
> vlans added to a port i.e both use XXXLINK.

But let's either add a new flag or use -compressvlans to print it when monitoring/showing
link otherwise people who are monitoring only the port flags will start getting lists
with vlans. Even compressed these can still be quite long and confusing, especially
when monitoring.

> In general, the XXXLINK interface with these master devices (bridge,
> bond etc) continues to get messy. Recently started seeing events with
> devices claiming to be of KIND_slave etc on the bridge and bond devices;
> yet at the same time my wireless card events are also showing up on the
> bridge link even though it is not enslaved there..
> 
> cheers,
> jamal
> 
>> Can you please paste a sample default detailed output with your patch
>> with a few hundred vlans ?
>>
> 

^ permalink raw reply

* Re: [patch net] net: sched: fix memleak for chain zero
From: Cong Wang @ 2017-09-09 18:46 UTC (permalink / raw)
  To: David Miller
  Cc: Jiri Pirko, Linux Kernel Network Developers, Jamal Hadi Salim,
	Jakub Kicinski, mlxsw
In-Reply-To: <20170907.191800.218886770521884093.davem@davemloft.net>

On Thu, Sep 7, 2017 at 7:18 PM, David Miller <davem@davemloft.net> wrote:
>
> This doesn't apply cleanly any more, please respin.
>

Sigh, you applied this patch despite of strong objections from me.

I seriously doubt your tastes now, David. Fine, this code does not
deserve a good taste at all, let bugs stay there.

Good luck!

^ permalink raw reply

* Re: [PATCH v2 net] net: qualcomm: rmnet: Fix a double free
From: Subash Abhinov Kasiviswanathan @ 2017-09-09 18:59 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: David S. Miller, netdev, kernel-janitors
In-Reply-To: <20170909085803.zgfqhp23yt4ol3ka@mwanda>

On 2017-09-09 02:58, Dan Carpenter wrote:
> There is a typo here so we accidentally free "skb" instead of "skbn".
> It leads to a double free and a leak.  After discussing with Subash,
> it's better to just move the check before the allocation and avoid the
> need to free.
> 
> Fixes: ceed73a2cf4a ("drivers: net: ethernet: qualcomm: rmnet: Initial
> implementation")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Fix the leak as well.  Thanks Subash!
> 
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 557c9bf1a469..86b8c758f94e 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -84,6 +84,10 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff 
> *skb)
>  	if (((int)skb->len - (int)packet_len) < 0)
>  		return NULL;
> 
> +	/* Some hardware can send us empty frames. Catch them */
> +	if (ntohs(maph->pkt_len) == 0)
> +		return NULL;
> +
>  	skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC);
>  	if (!skbn)
>  		return NULL;
> @@ -94,11 +98,5 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff 
> *skb)
>  	memcpy(skbn->data, skb->data, packet_len);
>  	skb_pull(skb, packet_len);
> 
> -	/* Some hardware can send us empty frames. Catch them */
> -	if (ntohs(maph->pkt_len) == 0) {
> -		kfree_skb(skb);
> -		return NULL;
> -	}
> -
>  	return skbn;
>  }

Acked-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>


-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox