* RE: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Tristram.Ha @ 2017-09-08 18:40 UTC (permalink / raw)
To: andrew, muvarov
Cc: pavel, nathan.leigh.conrad, vivien.didelot, f.fainelli, netdev,
linux-kernel, Woojung.Huh
In-Reply-To: <20170908141225.GE25219@lunn.ch>
> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Friday, September 08, 2017 7:12 AM
> To: Maxim Uvarov
> Cc: Tristram Ha - C24268; Pavel Machek; Nathan Conrad; Vivien Didelot; Florian
> Fainelli; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
> drivers can be added
>
> On Fri, Sep 08, 2017 at 04:32:35PM +0300, Maxim Uvarov wrote:
> > 2017-09-08 0:54 GMT+03:00 Andrew Lunn <andrew@lunn.ch>:
> > >> -- compatible: For external switch chips, compatible string must be exactly
> one
> > >> - of: "microchip,ksz9477"
> > >> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> > >> + "microchip,ksz8795" for KSZ8795 chip,
> > >> + "microchip,ksz8794" for KSZ8794 chip,
> > >> + "microchip,ksz8765" for KSZ8765 chip,
> > >> + "microchip,ksz8895" for KSZ8895 chip,
> > >> + "microchip,ksz8864" for KSZ8864 chip,
> > >> + "microchip,ksz8873" for KSZ8873 chip,
> > >> + "microchip,ksz8863" for KSZ8863 chip,
> > >> + "microchip,ksz8463" for KSZ8463 chip
> > >
> >
> > all that chips have the same spi access to get chip id on probe(). I
> > prefer common microship,ksz-spi rather than somebody will always
> > maintain that list.
>
> The Marvell DSA driver is similar. The compatibility string tells you
> enough to go find the switch ID in the switch itself.
>
> I suppose this comes down to, is there going to be one SPI driver for
> all the devices, or lots of drivers? In general, DSA has one driver
> for lots of devices. The mv88e6xxx supports around 25 devices. The b53
> has around 17, etc.
>
> So i would suggest one driver supporting all the different devices.
>
There will be 5 drivers to support these devices:
ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
ksz8795.c - KSZ8795/KSZ8795/KSZ8765
ksz8895.c - KSZ8895/KSZ8864
ksz8863.c - KSZ8863/KSZ8873
ksz8463.c - KSZ8463
These chips have different SPI access mechanisms, MIB counter reading,
and register set. These can be combined into one single driver using
function pointers, at least for ksz8795/ksz8895/ksz8863/ksz8463. My
only concern is the memory footprint. The customer may not want a
big driver to cover all the switches while only one is used.
Out of topic I have a question to ask the community regarding the DSA
port creation:
Port 1 can be specified using the reg parameter specifying 0; port 2, 1;
and so on. The KSZ8794 is a variant of KSZ8795 with port 4 disabled.
So
lan1 = 0, lan2 = 1, lan3 = 2, cpu = 4.
But our company Marketing does not want to promote that fact but treat
KSZ8794 as a distinct product.
So
lan1 = 0, lan2 = 1, lan3 = 2, cpu = 3.
Is this okay to hide this information inside the driver? This is manageable
for KSZ8794 but for KSZ8864 the first port is disabled:
lan1 = 1, lan2 = 2, lan3 = 3, cpu = 4.
I am not sure whether DSA has or will have a way to display the port
mapping to regular users.
^ permalink raw reply
* Re: [PATCH RFC 5/5] Add KSZ8795 SPI driver
From: Andrew Lunn @ 2017-09-08 18:37 UTC (permalink / raw)
To: Tristram.Ha
Cc: pavel, muvarov, nathan.leigh.conrad, vivien.didelot, f.fainelli,
netdev, linux-kernel, Woojung.Huh
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41121E1C@CHN-SV-EXMX02.mchp-main.com>
> Sorry about the formatting. It seems my e-mail system needs to be checked
> to make sure it does not auto-format the contents again.
I've never seen issues like this with git send-email. Please use
it. Email problems generally happen with the client, not the
backend. What client did you use to send these patches?
> About the SPI access functions they are the same for each driver except the
> low level ksz_spi_read_reg and ksz_spi_write_reg. The dev_io_ops structure
> should contain only those 2 and ksz_spi_get and ksz_spi_set.
>
> But that requires changing ksz_spi.c. The idea was to keep the code of
> KSZ9477 driver with little change as possible while introducing another driver.
Maintainability is always the primary goal. If you need to change the
KSZ9477 code to make the whole more maintainable, do so. Just make it
lots of small, easy to review, obviously correct changes.
Andrew
^ permalink raw reply
* Re: [PATCH 0/9] Netfilter/IPVS fixes for net
From: David Miller @ 2017-09-08 18:36 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, netdev
In-Reply-To: <1504892748-1605-1-git-send-email-pablo@netfilter.org>
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Fri, 8 Sep 2017 19:45:39 +0200
> The following patchset contains Netfilter/IPVS fixes for your net tree,
> they are:
>
> 1) Fix SCTP connection setup when IPVS module is loaded and any scheduler
> is registered, from Xin Long.
>
> 2) Don't create a SCTP connection from SCTP ABORT packets, also from
> Xin Long.
>
> 3) WARN_ON() and drop packet, instead of BUG_ON() races when calling
> nf_nat_setup_info(). This is specifically a longstanding problem
> when br_netfilter with conntrack support is in place, patch from
> Florian Westphal.
>
> 4) Avoid softlock splats via iptables-restore, also from Florian.
>
> 5) Revert NAT hashtable conversion to rhashtable, semantics of rhlist
> are different from our simple NAT hashtable, this has been causing
> problems in the recent Linux kernel releases. From Florian.
>
> 6) Add per-bucket spinlock for NAT hashtable, so at least we restore
> one of the benefits we got from the previous rhashtable conversion.
>
> 7) Fix incorrect hashtable size in memory allocation in xt_hashlimit,
> from Zhizhou Tian.
>
> 8) Fix build/link problems with hashlimit and 32-bit arches, to address
> recent fallout from a new hashlimit mode, from Vishwanath Pai.
Pulled, thanks Pablo.
^ permalink raw reply
* RE: [PATCH RFC 3/5] Add KSZ8795 switch driver
From: Woojung.Huh @ 2017-09-08 18:35 UTC (permalink / raw)
To: andrew, Tristram.Ha
Cc: pavel, muvarov, nathan.leigh.conrad, vivien.didelot, f.fainelli,
netdev, linux-kernel, UNGLinuxDriver
In-Reply-To: <20170908183227.GJ25219@lunn.ch>
> > > > @@ -0,0 +1,2066 @@
> > > > +/*
> > > > + * Microchip KSZ8795 switch driver
> > > > + *
> > > > + * Copyright (C) 2017 Microchip Technology Inc.
> > > > + * Tristram Ha <Tristram.Ha@microchip.com>
> > > > + *
> > > > + * Permission to use, copy, modify, and/or distribute this software for
> any
> > > > + * purpose with or without fee is hereby granted, provided that the
> above
> > > > + * copyright notice and this permission notice appear in all copies.
> > > > + *
> > > > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS
> ALL
> > > WARRANTIES
> > > > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
> WARRANTIES
> > > OF
> > > > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR
> BE
> > > LIABLE FOR
> > > > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR
> ANY
> > > DAMAGES
> > > > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS,
> WHETHER
> > > IN AN
> > > > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> > > ARISING OUT OF
> > > > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS
> SOFTWARE.
> > > > + */
> > >
> > > This is not exactly GPL, right? But tagging below says it is
> > > GPL. Please fix one.
> > >
> >
> > This boilerplate paragraph was copied from the KSZ9477 driver, although I
> did
> > wonder why this was used.
>
> Hi Tristram
>
> Please can you talk to your legal people and see if this can be
> replaced with the standard GPL text?
This should be replaced to GPL. These text copied from drivers/net/dsa/b53/*.
Will submit patches of drivers/net/dsa/microchip/*
- Woojung
^ permalink raw reply
* Re: [PATCH RFC 3/5] Add KSZ8795 switch driver
From: Andrew Lunn @ 2017-09-08 18:32 UTC (permalink / raw)
To: Tristram.Ha
Cc: pavel, muvarov, nathan.leigh.conrad, vivien.didelot, f.fainelli,
netdev, linux-kernel, Woojung.Huh
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>
> > > @@ -0,0 +1,2066 @@
> > > +/*
> > > + * Microchip KSZ8795 switch driver
> > > + *
> > > + * Copyright (C) 2017 Microchip Technology Inc.
> > > + * Tristram Ha <Tristram.Ha@microchip.com>
> > > + *
> > > + * Permission to use, copy, modify, and/or distribute this software for any
> > > + * purpose with or without fee is hereby granted, provided that the above
> > > + * copyright notice and this permission notice appear in all copies.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
> > WARRANTIES
> > > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES
> > OF
> > > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE
> > LIABLE FOR
> > > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY
> > DAMAGES
> > > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER
> > IN AN
> > > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> > ARISING OUT OF
> > > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > > + */
> >
> > This is not exactly GPL, right? But tagging below says it is
> > GPL. Please fix one.
> >
>
> This boilerplate paragraph was copied from the KSZ9477 driver, although I did
> wonder why this was used.
Hi Tristram
Please can you talk to your legal people and see if this can be
replaced with the standard GPL text?
> > > + for (timeout = 1; timeout > 0; timeout--) {
> > > + ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> > > +
> > > + if (check & MIB_COUNTER_VALID) {
> > > + ksz_read32(dev, REG_IND_DATA_LO, &data);
> > > + if (addr < 2) {
> > > + u64 total;
> > > +
> > > + total = check & MIB_TOTAL_BYTES_H;
> > > + total <<= 32;
> > > + *cnt += total;
> > > + *cnt += data;
> > > + if (check & MIB_COUNTER_OVERFLOW) {
> > > + total = MIB_TOTAL_BYTES_H + 1;
> > > + total <<= 32;
> > > + *cnt += total;
> > > + }
> > > + } else {
> > > + if (check & MIB_COUNTER_OVERFLOW)
> > > + *cnt += MIB_PACKET_DROPPED + 1;
> > > + *cnt += data & MIB_PACKET_DROPPED;
> > > + }
> > > + break;
> > > + }
> > > + }
> >
> > Why do you need a loop here? This is quite strange code. (And you have
> > similar strangeness elsewhere. Please fix.)
> >
>
> The MIB_COUNTER_VALID bit may be invalid on first read, although in slow
> SPI speed it never happens. The timeout value should be increased to 2.
Maybe timeout is the wrong name? There is nothing to do with time
here.
> > > +static int valid_dyn_entry(struct ksz_device *dev, u8 *data)
> > > +{
> > > + int timeout = 100;
> > > +
> > > + do {
> > > + ksz_read8(dev, REG_IND_DATA_CHECK, data);
> > > + timeout--;
> > > + } while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);
> > > +
> > > + /* Entry is not ready for accessing. */
> > > + if (*data & DYNAMIC_MAC_TABLE_NOT_READY) {
> > > + return 1;
> > > + /* Entry is ready for accessing. */
> > > + } else {
> > > + ksz_read8(dev, REG_IND_DATA_8, data);
> > > +
> > > + /* There is no valid entry in the table. */
> > > + if (*data & DYNAMIC_MAC_TABLE_MAC_EMPTY)
> > > + return 2;
> > > + }
> > > + return 0;
> > > +}
> >
> > Normal calling convention is 0 / -ERROR, not 0,1,2.
> >
>
> This is an internal function that is not returning any error. It just reports
> different conditions so the calling function decides what to do.
Still, best practice is to use standard error codes.
Andrew
^ permalink raw reply
* Re: [PATCH net] ipv6: fix memory leak with multiple tables during netns destruction
From: Cong Wang @ 2017-09-08 18:24 UTC (permalink / raw)
To: Sabrina Dubroca; +Cc: Linux Kernel Network Developers
In-Reply-To: <204659713af1979c26b8defb2b52e1668b4e5aad.1504859062.git.sd@queasysnail.net>
On Fri, Sep 8, 2017 at 1:26 AM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> fib6_net_exit only frees the main and local tables. If another table was
> created with fib6_alloc_table, we leak it when the netns is destroyed.
>
> Fix this in the same way ip_fib_net_exit cleans up tables, by walking
> through the whole hashtable of fib6_table's. We can get rid of the
> special cases for local and main, since they're also part of the
> hashtable.
>
> Reproducer:
> ip netns add x
> ip -net x -6 rule add from 6003:1::/64 table 100
> ip netns del x
>
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Fixes: 58f09b78b730 ("[NETNS][IPV6] ip6_fib - make it per network namespace")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
> net/ipv6/ip6_fib.c | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index a3b5c163325f..8280172c806c 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -191,6 +191,12 @@ void rt6_free_pcpu(struct rt6_info *non_pcpu_rt)
> }
> EXPORT_SYMBOL_GPL(rt6_free_pcpu);
>
> +static void fib6_free_table(struct fib6_table *table)
> +{
> + inetpeer_invalidate_tree(&table->tb6_peers);
> + kfree(table);
> +}
> +
> static void fib6_link_table(struct net *net, struct fib6_table *tb)
> {
> unsigned int h;
> @@ -2022,15 +2028,22 @@ static int __net_init fib6_net_init(struct net *net)
>
> static void fib6_net_exit(struct net *net)
> {
> + unsigned int i;
> +
> rt6_ifdown(net, NULL);
> del_timer_sync(&net->ipv6.ip6_fib_timer);
>
> -#ifdef CONFIG_IPV6_MULTIPLE_TABLES
> - inetpeer_invalidate_tree(&net->ipv6.fib6_local_tbl->tb6_peers);
> - kfree(net->ipv6.fib6_local_tbl);
> -#endif
> - inetpeer_invalidate_tree(&net->ipv6.fib6_main_tbl->tb6_peers);
> - kfree(net->ipv6.fib6_main_tbl);
> + for (i = 0; i < FIB_TABLE_HASHSZ; i++) {
> + struct hlist_head *head = &net->ipv6.fib_table_hash[i];
> + struct hlist_node *tmp;
> + struct fib6_table *tb;
> +
> + hlist_for_each_entry_safe(tb, tmp, head, tb6_hlist) {
> + hlist_del(&tb->tb6_hlist);
hlist_del_rcu() ?
> + fib6_free_table(tb);
kfree_rcu() in fib6_free_table()?
> + }
> + }
> +
> kfree(net->ipv6.fib_table_hash);
> kfree(net->ipv6.rt6_stats);
> fib6_notifier_exit(net);
> --
> 2.14.1
>
^ permalink raw reply
* Re: [PATCH net] ipv6: fix memory leak with multiple tables during netns destruction
From: Eric Dumazet @ 2017-09-08 17:59 UTC (permalink / raw)
To: Sabrina Dubroca; +Cc: netdev
In-Reply-To: <204659713af1979c26b8defb2b52e1668b4e5aad.1504859062.git.sd@queasysnail.net>
On Fri, 2017-09-08 at 10:26 +0200, Sabrina Dubroca wrote:
> fib6_net_exit only frees the main and local tables. If another table was
> created with fib6_alloc_table, we leak it when the netns is destroyed.
>
> Fix this in the same way ip_fib_net_exit cleans up tables, by walking
> through the whole hashtable of fib6_table's. We can get rid of the
> special cases for local and main, since they're also part of the
> hashtable.
>
> Reproducer:
> ip netns add x
> ip -net x -6 rule add from 6003:1::/64 table 100
> ip netns del x
>
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Fixes: 58f09b78b730 ("[NETNS][IPV6] ip6_fib - make it per network namespace")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
> net/ipv6/ip6_fib.c | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index a3b5c163325f..8280172c806c 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -191,6 +191,12 @@ void rt6_free_pcpu(struct rt6_info *non_pcpu_rt)
> }
> EXPORT_SYMBOL_GPL(rt6_free_pcpu);
>
> +static void fib6_free_table(struct fib6_table *table)
> +{
> + inetpeer_invalidate_tree(&table->tb6_peers);
> + kfree(table);
> +}
> +
> static void fib6_link_table(struct net *net, struct fib6_table *tb)
> {
> unsigned int h;
> @@ -2022,15 +2028,22 @@ static int __net_init fib6_net_init(struct net *net)
>
> static void fib6_net_exit(struct net *net)
> {
> + unsigned int i;
> +
> rt6_ifdown(net, NULL);
> del_timer_sync(&net->ipv6.ip6_fib_timer);
>
> -#ifdef CONFIG_IPV6_MULTIPLE_TABLES
> - inetpeer_invalidate_tree(&net->ipv6.fib6_local_tbl->tb6_peers);
> - kfree(net->ipv6.fib6_local_tbl);
> -#endif
> - inetpeer_invalidate_tree(&net->ipv6.fib6_main_tbl->tb6_peers);
> - kfree(net->ipv6.fib6_main_tbl);
> + for (i = 0; i < FIB_TABLE_HASHSZ; i++) {
should not this be FIB6_TABLE_HASHSZ ?
> + struct hlist_head *head = &net->ipv6.fib_table_hash[i];
> + struct hlist_node *tmp;
> + struct fib6_table *tb;
> +
> + hlist_for_each_entry_safe(tb, tmp, head, tb6_hlist) {
> + hlist_del(&tb->tb6_hlist);
> + fib6_free_table(tb);
> + }
> + }
> +
> kfree(net->ipv6.fib_table_hash);
> kfree(net->ipv6.rt6_stats);
> fib6_notifier_exit(net);
^ permalink raw reply
* RE: [PATCH RFC 3/5] Add KSZ8795 switch driver
From: Tristram.Ha @ 2017-09-08 17:54 UTC (permalink / raw)
To: pavel
Cc: andrew, muvarov, nathan.leigh.conrad, vivien.didelot, f.fainelli,
netdev, linux-kernel, Woojung.Huh
In-Reply-To: <20170908091856.GB17738@amd>
> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz]
> Sent: Friday, September 08, 2017 2:19 AM
> 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
>
> On Thu 2017-09-07 21:17:16, Tristram.Ha@microchip.com wrote:
> > From: Tristram Ha <Tristram.Ha@microchip.com>
> >
> > Add KSZ8795 switch support with function code.
>
> English? "Add KSZ8795 switch support." ?
>
> > Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> > ---
> > diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> > new file mode 100644
> > index 0000000..e4d4e6a
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -0,0 +1,2066 @@
> > +/*
> > + * Microchip KSZ8795 switch driver
> > + *
> > + * Copyright (C) 2017 Microchip Technology Inc.
> > + * Tristram Ha <Tristram.Ha@microchip.com>
> > + *
> > + * Permission to use, copy, modify, and/or distribute this software for any
> > + * purpose with or without fee is hereby granted, provided that the above
> > + * copyright notice and this permission notice appear in all copies.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
> WARRANTIES
> > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES
> OF
> > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE
> LIABLE FOR
> > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY
> DAMAGES
> > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER
> IN AN
> > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> ARISING OUT OF
> > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > + */
>
> This is not exactly GPL, right? But tagging below says it is
> GPL. Please fix one.
>
This boilerplate paragraph was copied from the KSZ9477 driver, although I did
wonder why this was used.
> > +static int ksz_reset_switch(struct ksz_device *dev)
> > +{
> > + /* reset switch */
> > + ksz_write8(dev, REG_POWER_MANAGEMENT_1,
> > + SW_SOFTWARE_POWER_DOWN <<
> SW_POWER_MANAGEMENT_MODE_S);
> > + ksz_write8(dev, REG_POWER_MANAGEMENT_1, 0);
> > +
> > + return 0;
> > +}
>
> This is going to be same in other drivers, right?
>
The switch reset is different in each chip, although KSZ8795 and
KSZ8895 use the same mechanism.
> > +
> > + for (timeout = 1; timeout > 0; timeout--) {
> > + ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> > +
> > + if (check & MIB_COUNTER_VALID) {
> > + ksz_read32(dev, REG_IND_DATA_LO, &data);
> > + if (addr < 2) {
> > + u64 total;
> > +
> > + total = check & MIB_TOTAL_BYTES_H;
> > + total <<= 32;
> > + *cnt += total;
> > + *cnt += data;
> > + if (check & MIB_COUNTER_OVERFLOW) {
> > + total = MIB_TOTAL_BYTES_H + 1;
> > + total <<= 32;
> > + *cnt += total;
> > + }
> > + } else {
> > + if (check & MIB_COUNTER_OVERFLOW)
> > + *cnt += MIB_PACKET_DROPPED + 1;
> > + *cnt += data & MIB_PACKET_DROPPED;
> > + }
> > + break;
> > + }
> > + }
>
> Why do you need a loop here? This is quite strange code. (And you have
> similar strangeness elsewhere. Please fix.)
>
The MIB_COUNTER_VALID bit may be invalid on first read, although in slow
SPI speed it never happens. The timeout value should be increased to 2.
> > +static int valid_dyn_entry(struct ksz_device *dev, u8 *data)
> > +{
> > + int timeout = 100;
> > +
> > + do {
> > + ksz_read8(dev, REG_IND_DATA_CHECK, data);
> > + timeout--;
> > + } while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);
> > +
> > + /* Entry is not ready for accessing. */
> > + if (*data & DYNAMIC_MAC_TABLE_NOT_READY) {
> > + return 1;
> > + /* Entry is ready for accessing. */
> > + } else {
> > + ksz_read8(dev, REG_IND_DATA_8, data);
> > +
> > + /* There is no valid entry in the table. */
> > + if (*data & DYNAMIC_MAC_TABLE_MAC_EMPTY)
> > + return 2;
> > + }
> > + return 0;
> > +}
>
> Normal calling convention is 0 / -ERROR, not 0,1,2.
>
This is an internal function that is not returning any error. It just reports
different conditions so the calling function decides what to do.
> > +static void ksz_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
> > +{
> > + struct ksz_port *port;
> > + u8 ctrl;
> > + u8 restart;
> > + u8 link;
> > + u8 speed;
> > + u8 force;
> > + u8 p = phy;
> > + u16 data = 0;
> > + int processed = true;
> > +
> > + port = &dev->ports[p];
> > + switch (reg) {
> > + case PHY_REG_CTRL:
> > + ksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);
> > + ksz_pread8(dev, p, P_NEG_RESTART_CTRL, &restart);
> > + ksz_pread8(dev, p, P_SPEED_STATUS, &speed);
> > + ksz_pread8(dev, p, P_FORCE_CTRL, &force);
> > + if (restart & PORT_PHY_LOOPBACK)
> > + data |= PHY_LOOPBACK;
> > + if (force & PORT_FORCE_100_MBIT)
> > + data |= PHY_SPEED_100MBIT;
> > + if (!(force & PORT_AUTO_NEG_DISABLE))
> > + data |= PHY_AUTO_NEG_ENABLE;
> > + if (restart & PORT_POWER_DOWN)
> > + data |= PHY_POWER_DOWN;
> > + if (restart & PORT_AUTO_NEG_RESTART)
> > + data |= PHY_AUTO_NEG_RESTART;
> > + if (force & PORT_FORCE_FULL_DUPLEX)
> > + data |= PHY_FULL_DUPLEX;
> > + if (speed & PORT_HP_MDIX)
> > + data |= PHY_HP_MDIX;
> > + if (restart & PORT_FORCE_MDIX)
> > + data |= PHY_FORCE_MDIX;
> > + if (restart & PORT_AUTO_MDIX_DISABLE)
> > + data |= PHY_AUTO_MDIX_DISABLE;
> > + if (restart & PORT_TX_DISABLE)
> > + data |= PHY_TRANSMIT_DISABLE;
> > + if (restart & PORT_LED_OFF)
> > + data |= PHY_LED_DISABLE;
> > + break;
> > + case PHY_REG_STATUS:
> > + ksz_pread8(dev, p, P_LINK_STATUS, &link);
> > + ksz_pread8(dev, p, P_SPEED_STATUS, &speed);
> > + data = PHY_100BTX_FD_CAPABLE |
> > + PHY_100BTX_CAPABLE |
> > + PHY_10BT_FD_CAPABLE |
> > + PHY_10BT_CAPABLE |
> > + PHY_AUTO_NEG_CAPABLE;
> > + if (link & PORT_AUTO_NEG_COMPLETE)
> > + data |= PHY_AUTO_NEG_ACKNOWLEDGE;
> > + if (link & PORT_STAT_LINK_GOOD) {
> > + data |= PHY_LINK_STATUS;
> > + if (!port->link_up) {
> > + port->link_up = 1;
> > + dev->live_ports |= (1 << phy) & dev->on_ports;
> > + }
> > + } else if (port->link_up) {
> > + port->link_down = 1;
> > + port->link_up = 0;
> > + dev->live_ports &= ~(1 << phy);
> > + }
> > + break;
> > + case PHY_REG_ID_1:
> > + data = KSZ8795_ID_HI;
> > + break;
> > + case PHY_REG_ID_2:
> > + data = KSZ8795_ID_LO;
> > + break;
> > + case PHY_REG_AUTO_NEGOTIATION:
> > + ksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);
> > + data = PHY_AUTO_NEG_802_3;
> > + if (ctrl & PORT_AUTO_NEG_SYM_PAUSE)
> > + data |= PHY_AUTO_NEG_SYM_PAUSE;
> > + if (ctrl & PORT_AUTO_NEG_100BTX_FD)
> > + data |= PHY_AUTO_NEG_100BTX_FD;
> > + if (ctrl & PORT_AUTO_NEG_100BTX)
> > + data |= PHY_AUTO_NEG_100BTX;
> > + if (ctrl & PORT_AUTO_NEG_10BT_FD)
> > + data |= PHY_AUTO_NEG_10BT_FD;
> > + if (ctrl & PORT_AUTO_NEG_10BT)
> > + data |= PHY_AUTO_NEG_10BT;
> > + break;
> > + case PHY_REG_REMOTE_CAPABILITY:
> > + ksz_pread8(dev, p, P_REMOTE_STATUS, &link);
> > + data = PHY_AUTO_NEG_802_3;
> > + if (link & PORT_REMOTE_SYM_PAUSE)
> > + data |= PHY_AUTO_NEG_SYM_PAUSE;
> > + if (link & PORT_REMOTE_100BTX_FD)
> > + data |= PHY_AUTO_NEG_100BTX_FD;
> > + if (link & PORT_REMOTE_100BTX)
> > + data |= PHY_AUTO_NEG_100BTX;
> > + if (link & PORT_REMOTE_10BT_FD)
> > + data |= PHY_AUTO_NEG_10BT_FD;
> > + if (link & PORT_REMOTE_10BT)
> > + data |= PHY_AUTO_NEG_10BT;
> > + break;
> > + 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.
^ permalink raw reply
* Re: [netfilter-core] [PATCH] netfilter: nat: constify rhashtable_params
From: Pablo Neira Ayuso @ 2017-09-08 17:47 UTC (permalink / raw)
To: Arvind Yadav
Cc: netdev, linux-kernel, coreteam, netfilter-devel, kadlec, davem
In-Reply-To: <20170908114630.GA13575@salvia>
On Fri, Sep 08, 2017 at 01:46:30PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Aug 30, 2017 at 05:18:04PM +0530, Arvind Yadav wrote:
> > rhashtable_params are not supposed to change at runtime. All
> > Functions rhashtable_* working with const rhashtable_params
> > provided by <linux/rhashtable.h>. So mark the non-const structs
> > as const.
>
> Applied to nf, thanks.
Actually, I tossed this. NAT rhashtable has been reverted, so no need
for this constification anymore.
^ permalink raw reply
* Re: [PATCH] can: check for null sk before deferencing it via the call to sock_net
From: Oliver Hartkopp @ 2017-09-08 17:46 UTC (permalink / raw)
To: Colin King, Marc Kleine-Budde, David S . Miller, linux-can,
netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20170908150235.2931-1-colin.king@canonical.com>
On 09/08/2017 05:02 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The assignment of net via call sock_net will dereference sk. This
> is performed before a sanity null check on sk, so there could be
> a potential null dereference on the sock_net call if sk is null.
> Fix this by assigning net after the sk null check. Also replace
> the sk == NULL with the more usual !sk idiom.
>
> Detected by CoverityScan CID#1431862 ("Dereference before null check")
>
> Fixes: 384317ef4187 ("can: network namespace support for CAN_BCM protocol")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Thanks Collin!
> ---
> net/can/bcm.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 47a8748d953a..a3791674b8ce 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -1493,13 +1493,14 @@ static int bcm_init(struct sock *sk)
> static int bcm_release(struct socket *sock)
> {
> struct sock *sk = sock->sk;
> - struct net *net = sock_net(sk);
> + struct net *net;
> struct bcm_sock *bo;
> struct bcm_op *op, *next;
>
> - if (sk == NULL)
> + if (!sk)
> return 0;
>
> + net = sock_net(sk);
> bo = bcm_sk(sk);
>
> /* remove bcm_ops, timer, rx_unregister(), etc. */
>
^ permalink raw reply
* [PATCH 9/9] netfilter: xt_hashlimit: fix build error caused by 64bit division
From: Pablo Neira Ayuso @ 2017-09-08 17:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1504892748-1605-1-git-send-email-pablo@netfilter.org>
From: Vishwanath Pai <vpai@akamai.com>
64bit division causes build/link errors on 32bit architectures. It
prints out error messages like:
ERROR: "__aeabi_uldivmod" [net/netfilter/xt_hashlimit.ko] undefined!
The value of avg passed through by userspace in BYTE mode cannot exceed
U32_MAX. Which means 64bit division in user2rate_bytes is unnecessary.
To fix this I have changed the type of param 'user' to u32.
Since anything greater than U32_MAX is an invalid input we error out in
hashlimit_mt_check_common() when this is the case.
Changes in v2:
Making return type as u32 would cause an overflow for small
values of 'user' (for example 2, 3 etc). To avoid this I bumped up
'r' to u64 again as well as the return type. This is OK since the
variable that stores the result is u64. We still avoid 64bit
division here since 'user' is u32.
Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
Signed-off-by: Vishwanath Pai <vpai@akamai.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/xt_hashlimit.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 962ea4a63d9f..5da8746f7b88 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -35,6 +35,7 @@
#include <linux/netfilter_ipv6/ip6_tables.h>
#include <linux/netfilter/xt_hashlimit.h>
#include <linux/mutex.h>
+#include <linux/kernel.h>
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Harald Welte <laforge@netfilter.org>");
@@ -527,12 +528,12 @@ static u64 user2rate(u64 user)
}
}
-static u64 user2rate_bytes(u64 user)
+static u64 user2rate_bytes(u32 user)
{
u64 r;
- r = user ? 0xFFFFFFFFULL / user : 0xFFFFFFFFULL;
- r = (r - 1) << 4;
+ r = user ? U32_MAX / user : U32_MAX;
+ r = (r - 1) << XT_HASHLIMIT_BYTE_SHIFT;
return r;
}
@@ -588,7 +589,8 @@ static void rateinfo_init(struct dsthash_ent *dh,
dh->rateinfo.prev_window = 0;
dh->rateinfo.current_rate = 0;
if (hinfo->cfg.mode & XT_HASHLIMIT_BYTES) {
- dh->rateinfo.rate = user2rate_bytes(hinfo->cfg.avg);
+ dh->rateinfo.rate =
+ user2rate_bytes((u32)hinfo->cfg.avg);
if (hinfo->cfg.burst)
dh->rateinfo.burst =
hinfo->cfg.burst * dh->rateinfo.rate;
@@ -870,7 +872,7 @@ static int hashlimit_mt_check_common(const struct xt_mtchk_param *par,
/* Check for overflow. */
if (revision >= 3 && cfg->mode & XT_HASHLIMIT_RATE_MATCH) {
- if (cfg->avg == 0) {
+ if (cfg->avg == 0 || cfg->avg > U32_MAX) {
pr_info("hashlimit invalid rate\n");
return -ERANGE;
}
--
2.1.4
^ permalink raw reply related
* [PATCH 8/9] netfilter: xt_hashlimit: alloc hashtable with right size
From: Pablo Neira Ayuso @ 2017-09-08 17:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1504892748-1605-1-git-send-email-pablo@netfilter.org>
From: Zhizhou Tian <zhizhou.tian@gmail.com>
struct xt_byteslimit_htable used hlist_head, but memory allocation is
done through sizeof(struct list_head).
Signed-off-by: Zhizhou Tian <zhizhou.tian@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/xt_hashlimit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 10d48234f5f4..962ea4a63d9f 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -279,7 +279,7 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
size = cfg->size;
} else {
size = (totalram_pages << PAGE_SHIFT) / 16384 /
- sizeof(struct list_head);
+ sizeof(struct hlist_head);
if (totalram_pages > 1024 * 1024 * 1024 / PAGE_SIZE)
size = 8192;
if (size < 16)
@@ -287,7 +287,7 @@ static int htable_create(struct net *net, struct hashlimit_cfg3 *cfg,
}
/* FIXME: don't use vmalloc() here or anywhere else -HW */
hinfo = vmalloc(sizeof(struct xt_hashlimit_htable) +
- sizeof(struct list_head) * size);
+ sizeof(struct hlist_head) * size);
if (hinfo == NULL)
return -ENOMEM;
*out_hinfo = hinfo;
--
2.1.4
^ permalink raw reply related
* [PATCH 5/9] netfilter: nat: Revert "netfilter: nat: convert nat bysrc hash to rhashtable"
From: Pablo Neira Ayuso @ 2017-09-08 17:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1504892748-1605-1-git-send-email-pablo@netfilter.org>
From: Florian Westphal <fw@strlen.de>
This reverts commit 870190a9ec9075205c0fa795a09fa931694a3ff1.
It was not a good idea. The custom hash table was a much better
fit for this purpose.
A fast lookup is not essential, in fact for most cases there is no lookup
at all because original tuple is not taken and can be used as-is.
What needs to be fast is insertion and deletion.
rhlist removal however requires a rhlist walk.
We can have thousands of entries in such a list if source port/addresses
are reused for multiple flows, if this happens removal requests are so
expensive that deletions of a few thousand flows can take several
seconds(!).
The advantages that we got from rhashtable are:
1) table auto-sizing
2) multiple locks
1) would be nice to have, but it is not essential as we have at
most one lookup per new flow, so even a million flows in the bysource
table are not a problem compared to current deletion cost.
2) is easy to add to custom hash table.
I tried to add hlist_node to rhlist to speed up rhltable_remove but this
isn't doable without changing semantics. rhltable_remove_fast will
check that the to-be-deleted object is part of the table and that
requires a list walk that we want to avoid.
Furthermore, using hlist_node increases size of struct rhlist_head, which
in turn increases nf_conn size.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=196821
Reported-by: Ivan Babrou <ibobrik@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_conntrack.h | 3 +-
include/net/netfilter/nf_nat.h | 1 -
net/netfilter/nf_nat_core.c | 130 ++++++++++++++---------------------
3 files changed, 54 insertions(+), 80 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index fdc9c64a1c94..8f3bd30511de 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -17,7 +17,6 @@
#include <linux/bitops.h>
#include <linux/compiler.h>
#include <linux/atomic.h>
-#include <linux/rhashtable.h>
#include <linux/netfilter/nf_conntrack_tcp.h>
#include <linux/netfilter/nf_conntrack_dccp.h>
@@ -77,7 +76,7 @@ struct nf_conn {
possible_net_t ct_net;
#if IS_ENABLED(CONFIG_NF_NAT)
- struct rhlist_head nat_bysource;
+ struct hlist_node nat_bysource;
#endif
/* all members below initialized via memset */
u8 __nfct_init_offset[0];
diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h
index 05c82a1a4267..b71701302e61 100644
--- a/include/net/netfilter/nf_nat.h
+++ b/include/net/netfilter/nf_nat.h
@@ -1,6 +1,5 @@
#ifndef _NF_NAT_H
#define _NF_NAT_H
-#include <linux/rhashtable.h>
#include <linux/netfilter_ipv4.h>
#include <linux/netfilter/nf_nat.h>
#include <net/netfilter/nf_conntrack_tuple.h>
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index dc3519cc7209..f090419f5f97 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -30,19 +30,17 @@
#include <net/netfilter/nf_conntrack_zones.h>
#include <linux/netfilter/nf_nat.h>
+static DEFINE_SPINLOCK(nf_nat_lock);
+
static DEFINE_MUTEX(nf_nat_proto_mutex);
static const struct nf_nat_l3proto __rcu *nf_nat_l3protos[NFPROTO_NUMPROTO]
__read_mostly;
static const struct nf_nat_l4proto __rcu **nf_nat_l4protos[NFPROTO_NUMPROTO]
__read_mostly;
-struct nf_nat_conn_key {
- const struct net *net;
- const struct nf_conntrack_tuple *tuple;
- const struct nf_conntrack_zone *zone;
-};
-
-static struct rhltable nf_nat_bysource_table;
+static struct hlist_head *nf_nat_bysource __read_mostly;
+static unsigned int nf_nat_htable_size __read_mostly;
+static unsigned int nf_nat_hash_rnd __read_mostly;
inline const struct nf_nat_l3proto *
__nf_nat_l3proto_find(u8 family)
@@ -118,17 +116,19 @@ int nf_xfrm_me_harder(struct net *net, struct sk_buff *skb, unsigned int family)
EXPORT_SYMBOL(nf_xfrm_me_harder);
#endif /* CONFIG_XFRM */
-static u32 nf_nat_bysource_hash(const void *data, u32 len, u32 seed)
+/* We keep an extra hash for each conntrack, for fast searching. */
+static unsigned int
+hash_by_src(const struct net *n, const struct nf_conntrack_tuple *tuple)
{
- const struct nf_conntrack_tuple *t;
- const struct nf_conn *ct = data;
+ unsigned int hash;
+
+ get_random_once(&nf_nat_hash_rnd, sizeof(nf_nat_hash_rnd));
- t = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
/* Original src, to ensure we map it consistently if poss. */
+ hash = jhash2((u32 *)&tuple->src, sizeof(tuple->src) / sizeof(u32),
+ tuple->dst.protonum ^ nf_nat_hash_rnd ^ net_hash_mix(n));
- seed ^= net_hash_mix(nf_ct_net(ct));
- return jhash2((const u32 *)&t->src, sizeof(t->src) / sizeof(u32),
- t->dst.protonum ^ seed);
+ return reciprocal_scale(hash, nf_nat_htable_size);
}
/* Is this tuple already taken? (not by us) */
@@ -184,28 +184,6 @@ same_src(const struct nf_conn *ct,
t->src.u.all == tuple->src.u.all);
}
-static int nf_nat_bysource_cmp(struct rhashtable_compare_arg *arg,
- const void *obj)
-{
- const struct nf_nat_conn_key *key = arg->key;
- const struct nf_conn *ct = obj;
-
- if (!same_src(ct, key->tuple) ||
- !net_eq(nf_ct_net(ct), key->net) ||
- !nf_ct_zone_equal(ct, key->zone, IP_CT_DIR_ORIGINAL))
- return 1;
-
- return 0;
-}
-
-static struct rhashtable_params nf_nat_bysource_params = {
- .head_offset = offsetof(struct nf_conn, nat_bysource),
- .obj_hashfn = nf_nat_bysource_hash,
- .obj_cmpfn = nf_nat_bysource_cmp,
- .nelem_hint = 256,
- .min_size = 1024,
-};
-
/* Only called for SRC manip */
static int
find_appropriate_src(struct net *net,
@@ -216,26 +194,22 @@ find_appropriate_src(struct net *net,
struct nf_conntrack_tuple *result,
const struct nf_nat_range *range)
{
+ unsigned int h = hash_by_src(net, tuple);
const struct nf_conn *ct;
- struct nf_nat_conn_key key = {
- .net = net,
- .tuple = tuple,
- .zone = zone
- };
- struct rhlist_head *hl, *h;
-
- hl = rhltable_lookup(&nf_nat_bysource_table, &key,
- nf_nat_bysource_params);
- rhl_for_each_entry_rcu(ct, h, hl, nat_bysource) {
- nf_ct_invert_tuplepr(result,
- &ct->tuplehash[IP_CT_DIR_REPLY].tuple);
- result->dst = tuple->dst;
-
- if (in_range(l3proto, l4proto, result, range))
- return 1;
+ hlist_for_each_entry_rcu(ct, &nf_nat_bysource[h], nat_bysource) {
+ if (same_src(ct, tuple) &&
+ net_eq(net, nf_ct_net(ct)) &&
+ nf_ct_zone_equal(ct, zone, IP_CT_DIR_ORIGINAL)) {
+ /* Copy source part from reply tuple. */
+ nf_ct_invert_tuplepr(result,
+ &ct->tuplehash[IP_CT_DIR_REPLY].tuple);
+ result->dst = tuple->dst;
+
+ if (in_range(l3proto, l4proto, result, range))
+ return 1;
+ }
}
-
return 0;
}
@@ -408,6 +382,7 @@ nf_nat_setup_info(struct nf_conn *ct,
const struct nf_nat_range *range,
enum nf_nat_manip_type maniptype)
{
+ struct net *net = nf_ct_net(ct);
struct nf_conntrack_tuple curr_tuple, new_tuple;
/* Can't setup nat info for confirmed ct. */
@@ -449,19 +424,14 @@ nf_nat_setup_info(struct nf_conn *ct,
}
if (maniptype == NF_NAT_MANIP_SRC) {
- struct nf_nat_conn_key key = {
- .net = nf_ct_net(ct),
- .tuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
- .zone = nf_ct_zone(ct),
- };
- int err;
-
- err = rhltable_insert_key(&nf_nat_bysource_table,
- &key,
- &ct->nat_bysource,
- nf_nat_bysource_params);
- if (err)
- return NF_DROP;
+ unsigned int srchash;
+
+ srchash = hash_by_src(net,
+ &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
+ spin_lock_bh(&nf_nat_lock);
+ hlist_add_head_rcu(&ct->nat_bysource,
+ &nf_nat_bysource[srchash]);
+ spin_unlock_bh(&nf_nat_lock);
}
/* It's done. */
@@ -570,8 +540,9 @@ static int nf_nat_proto_clean(struct nf_conn *ct, void *data)
* will delete entry from already-freed table.
*/
clear_bit(IPS_SRC_NAT_DONE_BIT, &ct->status);
- rhltable_remove(&nf_nat_bysource_table, &ct->nat_bysource,
- nf_nat_bysource_params);
+ spin_lock_bh(&nf_nat_lock);
+ hlist_del_rcu(&ct->nat_bysource);
+ spin_unlock_bh(&nf_nat_lock);
/* don't delete conntrack. Although that would make things a lot
* simpler, we'd end up flushing all conntracks on nat rmmod.
@@ -699,9 +670,11 @@ EXPORT_SYMBOL_GPL(nf_nat_l3proto_unregister);
/* No one using conntrack by the time this called. */
static void nf_nat_cleanup_conntrack(struct nf_conn *ct)
{
- if (ct->status & IPS_SRC_NAT_DONE)
- rhltable_remove(&nf_nat_bysource_table, &ct->nat_bysource,
- nf_nat_bysource_params);
+ if (ct->status & IPS_SRC_NAT_DONE) {
+ spin_lock_bh(&nf_nat_lock);
+ hlist_del_rcu(&ct->nat_bysource);
+ spin_unlock_bh(&nf_nat_lock);
+ }
}
static struct nf_ct_ext_type nat_extend __read_mostly = {
@@ -825,13 +798,16 @@ static int __init nf_nat_init(void)
{
int ret;
- ret = rhltable_init(&nf_nat_bysource_table, &nf_nat_bysource_params);
- if (ret)
- return ret;
+ /* Leave them the same for the moment. */
+ nf_nat_htable_size = nf_conntrack_htable_size;
+
+ nf_nat_bysource = nf_ct_alloc_hashtable(&nf_nat_htable_size, 0);
+ if (!nf_nat_bysource)
+ return -ENOMEM;
ret = nf_ct_extend_register(&nat_extend);
if (ret < 0) {
- rhltable_destroy(&nf_nat_bysource_table);
+ nf_ct_free_hashtable(nf_nat_bysource, nf_nat_htable_size);
printk(KERN_ERR "nf_nat_core: Unable to register extension\n");
return ret;
}
@@ -865,8 +841,8 @@ static void __exit nf_nat_cleanup(void)
for (i = 0; i < NFPROTO_NUMPROTO; i++)
kfree(nf_nat_l4protos[i]);
-
- rhltable_destroy(&nf_nat_bysource_table);
+ synchronize_net();
+ nf_ct_free_hashtable(nf_nat_bysource, nf_nat_htable_size);
}
MODULE_LICENSE("GPL");
--
2.1.4
^ permalink raw reply related
* [PATCH 6/9] netfilter: nat: use keyed locks
From: Pablo Neira Ayuso @ 2017-09-08 17:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1504892748-1605-1-git-send-email-pablo@netfilter.org>
From: Florian Westphal <fw@strlen.de>
no need to serialize on a single lock, we can partition the table and
add/delete in parallel to different slots.
This restores one of the advantages that got lost with the rhlist
revert.
Cc: Ivan Babrou <ibobrik@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_nat_core.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index f090419f5f97..f393a7086025 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -30,7 +30,7 @@
#include <net/netfilter/nf_conntrack_zones.h>
#include <linux/netfilter/nf_nat.h>
-static DEFINE_SPINLOCK(nf_nat_lock);
+static spinlock_t nf_nat_locks[CONNTRACK_LOCKS];
static DEFINE_MUTEX(nf_nat_proto_mutex);
static const struct nf_nat_l3proto __rcu *nf_nat_l3protos[NFPROTO_NUMPROTO]
@@ -425,13 +425,15 @@ nf_nat_setup_info(struct nf_conn *ct,
if (maniptype == NF_NAT_MANIP_SRC) {
unsigned int srchash;
+ spinlock_t *lock;
srchash = hash_by_src(net,
&ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
- spin_lock_bh(&nf_nat_lock);
+ lock = &nf_nat_locks[srchash % ARRAY_SIZE(nf_nat_locks)];
+ spin_lock_bh(lock);
hlist_add_head_rcu(&ct->nat_bysource,
&nf_nat_bysource[srchash]);
- spin_unlock_bh(&nf_nat_lock);
+ spin_unlock_bh(lock);
}
/* It's done. */
@@ -525,6 +527,16 @@ static int nf_nat_proto_remove(struct nf_conn *i, void *data)
return i->status & IPS_NAT_MASK ? 1 : 0;
}
+static void __nf_nat_cleanup_conntrack(struct nf_conn *ct)
+{
+ unsigned int h;
+
+ h = hash_by_src(nf_ct_net(ct), &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
+ spin_lock_bh(&nf_nat_locks[h % ARRAY_SIZE(nf_nat_locks)]);
+ hlist_del_rcu(&ct->nat_bysource);
+ spin_unlock_bh(&nf_nat_locks[h % ARRAY_SIZE(nf_nat_locks)]);
+}
+
static int nf_nat_proto_clean(struct nf_conn *ct, void *data)
{
if (nf_nat_proto_remove(ct, data))
@@ -540,9 +552,7 @@ static int nf_nat_proto_clean(struct nf_conn *ct, void *data)
* will delete entry from already-freed table.
*/
clear_bit(IPS_SRC_NAT_DONE_BIT, &ct->status);
- spin_lock_bh(&nf_nat_lock);
- hlist_del_rcu(&ct->nat_bysource);
- spin_unlock_bh(&nf_nat_lock);
+ __nf_nat_cleanup_conntrack(ct);
/* don't delete conntrack. Although that would make things a lot
* simpler, we'd end up flushing all conntracks on nat rmmod.
@@ -670,11 +680,8 @@ EXPORT_SYMBOL_GPL(nf_nat_l3proto_unregister);
/* No one using conntrack by the time this called. */
static void nf_nat_cleanup_conntrack(struct nf_conn *ct)
{
- if (ct->status & IPS_SRC_NAT_DONE) {
- spin_lock_bh(&nf_nat_lock);
- hlist_del_rcu(&ct->nat_bysource);
- spin_unlock_bh(&nf_nat_lock);
- }
+ if (ct->status & IPS_SRC_NAT_DONE)
+ __nf_nat_cleanup_conntrack(ct);
}
static struct nf_ct_ext_type nat_extend __read_mostly = {
@@ -796,10 +803,12 @@ static struct nf_ct_helper_expectfn follow_master_nat = {
static int __init nf_nat_init(void)
{
- int ret;
+ int ret, i;
/* Leave them the same for the moment. */
nf_nat_htable_size = nf_conntrack_htable_size;
+ if (nf_nat_htable_size < ARRAY_SIZE(nf_nat_locks))
+ nf_nat_htable_size = ARRAY_SIZE(nf_nat_locks);
nf_nat_bysource = nf_ct_alloc_hashtable(&nf_nat_htable_size, 0);
if (!nf_nat_bysource)
@@ -812,6 +821,9 @@ static int __init nf_nat_init(void)
return ret;
}
+ for (i = 0; i < ARRAY_SIZE(nf_nat_locks); i++)
+ spin_lock_init(&nf_nat_locks[i]);
+
nf_ct_helper_expectfn_register(&follow_master_nat);
BUG_ON(nfnetlink_parse_nat_setup_hook != NULL);
--
2.1.4
^ permalink raw reply related
* [PATCH 4/9] netfilter: xtables: add scheduling opportunity in get_counters
From: Pablo Neira Ayuso @ 2017-09-08 17:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1504892748-1605-1-git-send-email-pablo@netfilter.org>
From: Florian Westphal <fw@strlen.de>
There are reports about spurious softlockups during iptables-restore, a
backtrace i saw points at get_counters -- it uses a sequence lock and also
has unbounded restart loop.
Signed-off-by: Florian Westphal <fw@strlen.de>
Acked-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/ipv4/netfilter/arp_tables.c | 1 +
net/ipv4/netfilter/ip_tables.c | 1 +
net/ipv6/netfilter/ip6_tables.c | 1 +
3 files changed, 3 insertions(+)
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index e04457198f93..9e2770fd00be 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -629,6 +629,7 @@ static void get_counters(const struct xt_table_info *t,
ADD_COUNTER(counters[i], bcnt, pcnt);
++i;
+ cond_resched();
}
}
}
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 576cba2b57e9..39286e543ee6 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -776,6 +776,7 @@ get_counters(const struct xt_table_info *t,
ADD_COUNTER(counters[i], bcnt, pcnt);
++i; /* macro does multi eval of i */
+ cond_resched();
}
}
}
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 54b1e75eded1..01bd3ee5ebc6 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -795,6 +795,7 @@ get_counters(const struct xt_table_info *t,
ADD_COUNTER(counters[i], bcnt, pcnt);
++i;
+ cond_resched();
}
}
}
--
2.1.4
^ permalink raw reply related
* [PATCH 3/9] netfilter: nf_nat: don't bug when mapping already exists
From: Pablo Neira Ayuso @ 2017-09-08 17:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1504892748-1605-1-git-send-email-pablo@netfilter.org>
From: Florian Westphal <fw@strlen.de>
It seems preferrable to limp along if we have a conflicting mapping,
its certainly better than a BUG().
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_nat_core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 40573aa6c133..dc3519cc7209 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -416,7 +416,9 @@ nf_nat_setup_info(struct nf_conn *ct,
WARN_ON(maniptype != NF_NAT_MANIP_SRC &&
maniptype != NF_NAT_MANIP_DST);
- BUG_ON(nf_nat_initialized(ct, maniptype));
+
+ if (WARN_ON(nf_nat_initialized(ct, maniptype)))
+ return NF_DROP;
/* What we've got will look like inverse of reply. Normally
* this is what is in the conntrack, except for prior
--
2.1.4
^ permalink raw reply related
* [PATCH 2/9] netfilter: ipvs: do not create conn for ABORT packet in sctp_conn_schedule
From: Pablo Neira Ayuso @ 2017-09-08 17:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1504892748-1605-1-git-send-email-pablo@netfilter.org>
From: Xin Long <lucien.xin@gmail.com>
There's no reason for ipvs to create a conn for an ABORT packet
even if sysctl_sloppy_sctp is set.
This patch is to accept it without creating a conn, just as ipvs
does for tcp's RST packet.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/ipvs/ip_vs_proto_sctp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
index 81f08198b125..57c8ee66491e 100644
--- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
@@ -25,7 +25,8 @@ sctp_conn_schedule(struct netns_ipvs *ipvs, int af, struct sk_buff *skb,
sch = skb_header_pointer(skb, iph->len + sizeof(_sctph),
sizeof(_schunkh), &_schunkh);
if (sch) {
- if (!(sysctl_sloppy_sctp(ipvs) ||
+ if (sch->type == SCTP_CID_ABORT ||
+ !(sysctl_sloppy_sctp(ipvs) ||
sch->type == SCTP_CID_INIT))
return 1;
ports = &sh->source;
--
2.1.4
^ permalink raw reply related
* [PATCH 1/9] netfilter: ipvs: fix the issue that sctp_conn_schedule drops non-INIT packet
From: Pablo Neira Ayuso @ 2017-09-08 17:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1504892748-1605-1-git-send-email-pablo@netfilter.org>
From: Xin Long <lucien.xin@gmail.com>
Commit 5e26b1b3abce ("ipvs: support scheduling inverse and icmp SCTP
packets") changed to check packet type early. It introduced a side
effect: if it's not a INIT packet, ports will be set as NULL, and
the packet will be dropped later.
It caused that sctp couldn't create connection when ipvs module is
loaded and any scheduler is registered on server.
Li Shuang reproduced it by running the cmds on sctp server:
# ipvsadm -A -t 1.1.1.1:80 -s rr
# ipvsadm -D -t 1.1.1.1:80
then the server could't work any more.
This patch is to return 1 when it's not an INIT packet. It means ipvs
will accept it without creating a conn for it, just like what it does
for tcp.
Fixes: 5e26b1b3abce ("ipvs: support scheduling inverse and icmp SCTP packets")
Reported-by: Li Shuang <shuali@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/ipvs/ip_vs_proto_sctp.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
index e1efa446b305..81f08198b125 100644
--- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
@@ -24,9 +24,12 @@ sctp_conn_schedule(struct netns_ipvs *ipvs, int af, struct sk_buff *skb,
if (sh) {
sch = skb_header_pointer(skb, iph->len + sizeof(_sctph),
sizeof(_schunkh), &_schunkh);
- if (sch && (sch->type == SCTP_CID_INIT ||
- sysctl_sloppy_sctp(ipvs)))
+ if (sch) {
+ if (!(sysctl_sloppy_sctp(ipvs) ||
+ sch->type == SCTP_CID_INIT))
+ return 1;
ports = &sh->source;
+ }
}
} else {
ports = skb_header_pointer(
--
2.1.4
^ permalink raw reply related
* [PATCH 0/9] Netfilter/IPVS fixes for net
From: Pablo Neira Ayuso @ 2017-09-08 17:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
Hi David,
The following patchset contains Netfilter/IPVS fixes for your net tree,
they are:
1) Fix SCTP connection setup when IPVS module is loaded and any scheduler
is registered, from Xin Long.
2) Don't create a SCTP connection from SCTP ABORT packets, also from
Xin Long.
3) WARN_ON() and drop packet, instead of BUG_ON() races when calling
nf_nat_setup_info(). This is specifically a longstanding problem
when br_netfilter with conntrack support is in place, patch from
Florian Westphal.
4) Avoid softlock splats via iptables-restore, also from Florian.
5) Revert NAT hashtable conversion to rhashtable, semantics of rhlist
are different from our simple NAT hashtable, this has been causing
problems in the recent Linux kernel releases. From Florian.
6) Add per-bucket spinlock for NAT hashtable, so at least we restore
one of the benefits we got from the previous rhashtable conversion.
7) Fix incorrect hashtable size in memory allocation in xt_hashlimit,
from Zhizhou Tian.
8) Fix build/link problems with hashlimit and 32-bit arches, to address
recent fallout from a new hashlimit mode, from Vishwanath Pai.
You can pull these changes from:
git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git
Thanks!
----------------------------------------------------------------
The following changes since commit 8e0deed92406d93ae0365cb8a6134db5721e7aca:
tipc: remove unnecessary call to dev_net() (2017-09-06 21:25:52 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git HEAD
for you to fetch changes up to 90c4ae4e2c1da9f1eaf846136861af43d4c1ff34:
netfilter: xt_hashlimit: fix build error caused by 64bit division (2017-09-08 18:55:53 +0200)
----------------------------------------------------------------
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
Vishwanath Pai (1):
netfilter: xt_hashlimit: fix build error caused by 64bit division
Xin Long (2):
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
include/net/netfilter/nf_conntrack.h | 3 +-
include/net/netfilter/nf_nat.h | 1 -
net/ipv4/netfilter/arp_tables.c | 1 +
net/ipv4/netfilter/ip_tables.c | 1 +
net/ipv6/netfilter/ip6_tables.c | 1 +
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 ++--
9 files changed, 88 insertions(+), 91 deletions(-)
^ permalink raw reply
* [PATCH 7/9] netfilter: core: remove erroneous warn_on
From: Pablo Neira Ayuso @ 2017-09-08 17:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1504892748-1605-1-git-send-email-pablo@netfilter.org>
From: Florian Westphal <fw@strlen.de>
kernel test robot reported:
WARNING: CPU: 0 PID: 1244 at net/netfilter/core.c:218 __nf_hook_entries_try_shrink+0x49/0xcd
[..]
After allowing batching in nf_unregister_net_hooks its possible that an earlier
call to __nf_hook_entries_try_shrink already compacted the list.
If this happens we don't need to do anything.
Fixes: d3ad2c17b4047 ("netfilter: core: batch nf_unregister_net_hooks synchronize_net calls")
Reported-by: kernel test robot <xiaolong.ye@intel.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
Acked-by: Aaron Conole <aconole@bytheb.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 04fe25abc5f6..52cd2901a097 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -215,7 +215,7 @@ static void *__nf_hook_entries_try_shrink(struct nf_hook_entries __rcu **pp)
if (skip == hook_entries)
goto out_assign;
- if (WARN_ON(skip == 0))
+ if (skip == 0)
return NULL;
hook_entries -= skip;
--
2.1.4
^ permalink raw reply related
* RE: [PATCH RFC 5/5] Add KSZ8795 SPI driver
From: Tristram.Ha @ 2017-09-08 17:35 UTC (permalink / raw)
To: pavel
Cc: andrew, muvarov, nathan.leigh.conrad, vivien.didelot, f.fainelli,
netdev, linux-kernel, Woojung.Huh
In-Reply-To: <20170908092614.GB18365@amd>
> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz]
> Sent: Friday, September 08, 2017 2:26 AM
> 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 5/5] Add KSZ8795 SPI driver
>
> Hi!
>
>
> > +static int ksz_spi_read(struct ksz_device *dev, u32 reg, u8 *data,
> > + unsigned int len)
> > +{
> > + struct spi_device *spi = dev->priv;
> > +
> > + return ksz_spi_read_reg(spi, reg, data, len); }
> > +
> > +static int ksz_spi_read8(struct ksz_device *dev, u32 reg, u8 *val) {
> > + return ksz_spi_read(dev, reg, val, 1); }
> > +
> > +static int ksz_spi_read16(struct ksz_device *dev, u32 reg, u16 *val) {
> > + int ret = ksz_spi_read(dev, reg, (u8 *)val, 2);
> > +
> > + if (!ret)
> > + *val = be16_to_cpu(*val);
> > +
> > + return ret;
> > +}
>
> > +static int ksz_spi_read32(struct ksz_device *dev, u32 reg, u32 *val) {
> > + int ret = ksz_spi_read(dev, reg, (u8 *)val, 4);
> > +
> > + if (!ret)
> > + *val = be32_to_cpu(*val);
> > +
> > + return ret;
> > +}
>
> Please format according to CodingStyle. (Not only this.)
>
> And this will be common for more drivers. Can it go to a header file
> and be included...?
>
Sorry about the formatting. It seems my e-mail system needs to be checked
to make sure it does not auto-format the contents again.
About the SPI access functions they are the same for each driver except the
low level ksz_spi_read_reg and ksz_spi_write_reg. The dev_io_ops structure
should contain only those 2 and ksz_spi_get and ksz_spi_set.
But that requires changing ksz_spi.c. The idea was to keep the code of
KSZ9477 driver with little change as possible while introducing another driver.
The KSZ9477 driver will need to be updated with some of the code in KSZ8795
driver regarding port membership and MIB counter reading.
^ permalink raw reply
* Re: Use after free in __dst_destroy_metrics_generic
From: Eric Dumazet @ 2017-09-08 17:28 UTC (permalink / raw)
To: David Miller; +Cc: xiyou.wangcong, subashab, netdev, lorenzo
In-Reply-To: <20170908.101936.1889796764038070767.davem@davemloft.net>
On Fri, 2017-09-08 at 10:19 -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Fri, 08 Sep 2017 10:16:53 -0700
>
> > On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote:
> >> On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
> >> <subashab@codeaurora.org> wrote:
> >> > We are seeing a possible use after free in ip6_dst_destroy.
> >> >
> >> > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path
> >> > and allocated
> >> > to ion driver. ion driver has also freed it. Finally the memory is freed by
> >> > the
> >> > fib gc and crashes since it is already deallocated.
> >>
> >> Does the attach (compile-only) patch help anything?
> >>
> >> From my _quick_ glance, it seems we miss the refcnt'ing
> >> right in __dst_destroy_metrics_generic().
> >>
> >> Thanks!
> >
> > But 4.9 kernels do not have yet the DST_METRICS_REFCOUNTED thing,
> > since this was added in 4.12
>
> It was backported via -stable.
I hate working on lazy bug reports.
^ permalink raw reply
* Re: [PATCH v2 0/2] enable hires timer to timeout datagram socket
From: David Miller @ 2017-09-08 17:26 UTC (permalink / raw)
To: dwmw2
Cc: eduval, vallish, shuah, richardcochran, xiyou.wangcong, netdev,
linux-kernel, anchalag, dwmw
In-Reply-To: <1504891402.32080.5.camel@infradead.org>
From: David Woodhouse <dwmw2@infradead.org>
Date: Fri, 08 Sep 2017 18:23:22 +0100
> I don't know that anyone's ever tried saying "show me the chapter and
> verse of the documentation"
Do you know why I brought this up? Because the person I am replying
to told me that the syscall documentation should have suggested this
or that.
That's why.
So let's concentrate on the other aspects of my reply, ok?
^ permalink raw reply
* Re: [PATCH v2 0/2] enable hires timer to timeout datagram socket
From: David Woodhouse @ 2017-09-08 17:23 UTC (permalink / raw)
To: David Miller, eduval
Cc: vallish, shuah, richardcochran, xiyou.wangcong, netdev,
linux-kernel, anchalag, dwmw
In-Reply-To: <20170908.101657.2131282706895004921.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 1591 bytes --]
On Fri, 2017-09-08 at 10:16 -0700, David Miller wrote:
> From: Eduardo Valentin <eduval@amazon.com>
> Date: Fri, 8 Sep 2017 10:04:09 -0700
>
> >
> > However, this is a clear, the system call, from the net subsystem,
> > has changed in behavior across kernel versions. From application /
> > userspace perspective, changing the system call without clear
> > documentation or deprecation path, to me, looks like breaking
> > userspace, isn't it?
>
> Where is the chapter and verse of the system call documentation that
> guaranteed this level of timer granularity for you?
>
> Or were you simply relying upon implementation dependent behavior?
> I can't see anything which ever guarateed the granularity of timers
> to the extent upon which you were relying.
>
> And most importantly, letting the kernel have flexibility in this area
> is absolutely essential for various forms of optimizations and power
> savings.
The rule we normally use, typically enforced very shoutily by Linus, is
that *however* stupid userspace was to rely on something, if they *do*
rely on it then we shouldn't change it.
I don't know that anyone's ever tried saying "show me the chapter and
verse of the documentation" to Linus when he's in full rant mode, as he
tends to get in such discussions. You could try it, I suppose.
I don't think 'HZ==100' was documented per se either, was it? Perhaps
we *could* change that, after all? :)
(Not that I've actually looked at the patch or the userspace in
question yet, mind you. Just commenting on the absurdity of the
response.)
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]
^ permalink raw reply
* Re: WARNING: CPU: 2 PID: 4277 at lib/refcount.c:186
From: Cong Wang @ 2017-09-08 17:21 UTC (permalink / raw)
To: Shankara Pailoor
Cc: LKML, syzkaller, David Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, Linux Kernel Network Developers
In-Reply-To: <CAASgV=s63Mj5Ki-ep0pUrOpGonaaOZm3TEeENk+rjxA=koxPfg@mail.gmail.com>
(Cc'ing netdev)
On Fri, Sep 8, 2017 at 5:59 AM, Shankara Pailoor <sp3485@columbia.edu> wrote:
> Hi,
>
> I found a warning while fuzzing with Syzkaller on linux 4.13-rc7 on
> x86_64. The full stack trace is below:
>
> WARNING: CPU: 2 PID: 4277 at lib/refcount.c:186
> refcount_sub_and_test+0x167/0x1b0 lib/refcount.c:186
> Kernel panic - not syncing: panic_on_warn set ...
>
> CPU: 2 PID: 4277 Comm: syz-executor0 Not tainted 4.13.0-rc7 #3
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> Ubuntu-1.8.2-1ubuntu1 04/01/2014
> Call Trace:
> <IRQ>
> __dump_stack lib/dump_stack.c:16 [inline]
> dump_stack+0xf7/0x1aa lib/dump_stack.c:52
> panic+0x1ae/0x3a7 kernel/panic.c:180
> __warn+0x1c4/0x1d9 kernel/panic.c:541
> report_bug+0x211/0x2d0 lib/bug.c:183
> fixup_bug+0x40/0x90 arch/x86/kernel/traps.c:190
> do_trap_no_signal arch/x86/kernel/traps.c:224 [inline]
> do_trap+0x260/0x390 arch/x86/kernel/traps.c:273
> do_error_trap+0x118/0x340 arch/x86/kernel/traps.c:310
> do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:323
> invalid_op+0x18/0x20 arch/x86/entry/entry_64.S:846
> RIP: 0010:refcount_sub_and_test+0x167/0x1b0 lib/refcount.c:186
> RSP: 0018:ffff88006e006b60 EFLAGS: 00010286
> RAX: 0000000000000026 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: 0000000000000026 RSI: 1ffff1000dc00d2c RDI: ffffed000dc00d60
> RBP: ffff88006e006bf0 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 1ffff1000dc00d6d
> R13: 00000000ffffffff R14: 0000000000000001 R15: ffff88006ce9d340
> refcount_dec_and_test+0x1a/0x20 lib/refcount.c:211
> reqsk_put+0x71/0x2b0 include/net/request_sock.h:123
> tcp_v4_rcv+0x259e/0x2e20 net/ipv4/tcp_ipv4.c:1729
> ip_local_deliver_finish+0x2e2/0xba0 net/ipv4/ip_input.c:216
> NF_HOOK include/linux/netfilter.h:248 [inline]
> ip_local_deliver+0x1ce/0x6d0 net/ipv4/ip_input.c:257
> dst_input include/net/dst.h:477 [inline]
> ip_rcv_finish+0x8db/0x19c0 net/ipv4/ip_input.c:397
> NF_HOOK include/linux/netfilter.h:248 [inline]
> ip_rcv+0xc3f/0x17d0 net/ipv4/ip_input.c:488
> __netif_receive_skb_core+0x1fb7/0x31f0 net/core/dev.c:4298
> __netif_receive_skb+0x2c/0x1b0 net/core/dev.c:4336
> process_backlog+0x1c5/0x6d0 net/core/dev.c:5102
> napi_poll net/core/dev.c:5499 [inline]
> net_rx_action+0x6d3/0x14a0 net/core/dev.c:5565
> __do_softirq+0x2cb/0xb2d kernel/softirq.c:284
> do_softirq_own_stack+0x1c/0x30 arch/x86/entry/entry_64.S:898
> </IRQ>
> do_softirq.part.16+0x63/0x80 kernel/softirq.c:328
> do_softirq kernel/softirq.c:176 [inline]
> __local_bh_enable_ip+0x84/0x90 kernel/softirq.c:181
> local_bh_enable include/linux/bottom_half.h:31 [inline]
> rcu_read_unlock_bh include/linux/rcupdate.h:705 [inline]
> ip_finish_output2+0x8ad/0x1360 net/ipv4/ip_output.c:231
> ip_finish_output+0x74e/0xb80 net/ipv4/ip_output.c:317
> NF_HOOK_COND include/linux/netfilter.h:237 [inline]
> ip_output+0x1cc/0x850 net/ipv4/ip_output.c:405
> dst_output include/net/dst.h:471 [inline]
> ip_local_out+0x95/0x160 net/ipv4/ip_output.c:124
> ip_queue_xmit+0x8c6/0x1810 net/ipv4/ip_output.c:504
> tcp_transmit_skb+0x1963/0x3320 net/ipv4/tcp_output.c:1123
> tcp_send_ack.part.35+0x38c/0x620 net/ipv4/tcp_output.c:3575
> tcp_send_ack+0x49/0x60 net/ipv4/tcp_output.c:3545
> tcp_rcv_synsent_state_process net/ipv4/tcp_input.c:5795 [inline]
> tcp_rcv_state_process+0x4876/0x4b60 net/ipv4/tcp_input.c:5930
> tcp_v4_do_rcv+0x58a/0x820 net/ipv4/tcp_ipv4.c:1483
> sk_backlog_rcv include/net/sock.h:907 [inline]
> __release_sock+0x124/0x360 net/core/sock.c:2223
> release_sock+0xa4/0x2a0 net/core/sock.c:2715
> inet_wait_for_connect net/ipv4/af_inet.c:557 [inline]
> __inet_stream_connect+0x671/0xf00 net/ipv4/af_inet.c:643
> inet_stream_connect+0x58/0xa0 net/ipv4/af_inet.c:682
> SYSC_connect+0x204/0x470 net/socket.c:1628
> SyS_connect+0x24/0x30 net/socket.c:1609
> entry_SYSCALL_64_fastpath+0x18/0xad
> RIP: 0033:0x451e59
> RSP: 002b:00007f474843fc08 EFLAGS: 00000216 ORIG_RAX: 000000000000002a
> RAX: ffffffffffffffda RBX: 0000000000718000 RCX: 0000000000451e59
> RDX: 0000000000000010 RSI: 0000000020002000 RDI: 0000000000000007
> RBP: 0000000000000046 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000216 R12: 0000000000000000
> R13: 00007ffc040a0f8f R14: 00007f47484409c0 R15: 0000000000000000
>
>
>
>
> I found that the following program is able to reproduce the warning:
>
>
> Pastebin: https://pastebin.com/B75BdYKz
>
> Here are my configs: https://pastebin.com/zRYCXbak
>
> Regards,
> Shankara
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox