* Re: [PATCH iproute2 net-next] gre6: add collect metadata support
From: William Tu @ 2017-12-04 20:40 UTC (permalink / raw)
To: Gregory Rose; +Cc: Linux Kernel Network Developers
In-Reply-To: <277df688-0a71-bd3f-eb67-c706113bebd2@gmail.com>
On Mon, Dec 4, 2017 at 9:26 AM, Gregory Rose <gvrose8192@gmail.com> wrote:
> On 12/1/2017 3:03 PM, William Tu wrote:
>>
>> The patch adds 'external' option to support collect metadata
>> gre6 tunnel. Example of L3 and L2 gre device:
>> bash:~# ip link add dev ip6gre123 type ip6gre external
>> bash:~# ip link add dev ip6gretap123 type ip6gretap external
>
>
> Hi William,
>
> one question. Is there ever actually going to be a situation in which an
> ipv6 gre tunnel will go from
> external to not external? I'm wondering if the "no external" option is
> really needed since default is
> no and I'm not sure under what circumstances a tunnel would go from external
> to not external.
>
> Thanks,
>
> - Greg
good point. I don't think we should support setting a tunnel from
external to no external at run-time.
Let me remove the "noexternal" description in man page.
Thanks
William
^ permalink raw reply
* Re: [PATCH] tcp/dccp: CVE-2017-8824: use-after-free in DCCP code
From: Eric Dumazet @ 2017-12-04 20:41 UTC (permalink / raw)
To: simo.ghannam, netdev
In-Reply-To: <5a25afc4.919bdf0a.c917c.b05a@mx.google.com>
On Mon, 2017-12-04 at 21:27 +0100, simo.ghannam@gmail.com wrote:
> From: Mohamed Ghannam <simo.ghannam@gmail.com>
>
> Whenever the sock object is in DCCP_CLOSED state,
> dccp_disconnect() must free
> dccps_hc_tx_ccid and dccps_hc_rx_ccid and set to NULL.
>
> Signed-off-by: Mohamed Ghannam <simo.ghannam@gmail.com>
> ---
Small nit : white spaces in changelog where not needed :
Whenever the sock object is in DCCP_CLOSED state, dccp_disconnect()
must free dccps_hc_tx_ccid and dccps_hc_rx_ccid and set them to NULL.
Reviewed-by: Eric Dumazet <edumazet@google.com>
Thanks !
^ permalink raw reply
* RE: [Patch net v2] tipc: fix a null pointer deref on error path
From: Jon Maloy @ 2017-12-04 20:32 UTC (permalink / raw)
To: Jon Maloy, Cong Wang, David Miller
Cc: Linux Kernel Network Developers,
tipc-discussion@lists.sourceforge.net, Ying Xue
In-Reply-To: <AM4PR07MB171496B1708D7CA836C8C7519A3C0@AM4PR07MB1714.eurprd07.prod.outlook.com>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Jon Maloy
> Sent: Monday, December 04, 2017 14:50
> To: Cong Wang <xiyou.wangcong@gmail.com>; David Miller
> <davem@davemloft.net>
> Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; tipc-
> discussion@lists.sourceforge.net; Ying Xue <ying.xue@windriver.com>
> Subject: RE: [Patch net v2] tipc: fix a null pointer deref on error path
>
>
>
> > -----Original Message-----
> > From: Cong Wang [mailto:xiyou.wangcong@gmail.com]
> > Sent: Monday, December 04, 2017 14:41
> > To: David Miller <davem@davemloft.net>
> > Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; tipc-
> > discussion@lists.sourceforge.net; Jon Maloy <jon.maloy@ericsson.com>;
> > Ying Xue <ying.xue@windriver.com>
> > Subject: Re: [Patch net v2] tipc: fix a null pointer deref on error
> > path
> >
> > On Mon, Dec 4, 2017 at 11:23 AM, Cong Wang
> <xiyou.wangcong@gmail.com>
> > wrote:
> > > On Mon, Dec 4, 2017 at 10:57 AM, David Miller <davem@davemloft.net>
> > wrote:
> > >>
> > >> It looks like tipc_accept_from_sock() has a similar problem? The
> > >> tipc_close_conn() will get invoked indirectly from the
> > >> sock_release() path right?
> > >
> > > Not sure, the sock_release() in tipc_accept_from_sock() is for
> > > kernel_accept(), not for tipc_alloc_conn(). Or maybe it is hiding
> > > deep in the call chain that I miss?
> >
> > I see:
> >
> > tipc_release() -> tipc_sk_leave() -> tipc_group_delete()
> > -> tipc_topsrv_kern_unsubscr() -> tipc_close_conn()
> >
> > Seems on this path we do need to skip NULL too.
>
> You are right. The right solution is to just call conn_put() twice here.
> I already have a patch ready for this, but it is part of a series that needs more
> review.
> I should probably post it separately...
Well, calling conn_put() twice was ok in my series, but in the current upstream version it is not enough.
I will find a different short term solution.
///jon
>
> ///jon
^ permalink raw reply
* [PATCH] tcp/dccp: CVE-2017-8824: use-after-free in DCCP code
From: simo.ghannam @ 2017-12-04 20:27 UTC (permalink / raw)
To: netdev; +Cc: Mohamed Ghannam
From: Mohamed Ghannam <simo.ghannam@gmail.com>
Whenever the sock object is in DCCP_CLOSED state, dccp_disconnect() must free
dccps_hc_tx_ccid and dccps_hc_rx_ccid and set to NULL.
Signed-off-by: Mohamed Ghannam <simo.ghannam@gmail.com>
---
net/dccp/proto.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/dccp/proto.c b/net/dccp/proto.c
index b68168f..1814584 100644
--- a/net/dccp/proto.c
+++ b/net/dccp/proto.c
@@ -259,6 +259,7 @@ int dccp_disconnect(struct sock *sk, int flags)
{
struct inet_connection_sock *icsk = inet_csk(sk);
struct inet_sock *inet = inet_sk(sk);
+ struct dccp_sock *dp = dccp_sk(sk);
int err = 0;
const int old_state = sk->sk_state;
@@ -279,6 +280,9 @@ int dccp_disconnect(struct sock *sk, int flags)
dccp_clear_xmit_timers(sk);
+ ccid_hc_rx_delete(dp->dccps_hc_rx_ccid, sk);
+ ccid_hc_tx_delete(dp->dccps_hc_tx_ccid, sk);
+ dp->dccps_hc_rx_ccid = dp->dccps_hc_tx_ccid = NULL;
__skb_queue_purge(&sk->sk_receive_queue);
__skb_queue_purge(&sk->sk_write_queue);
if (sk->sk_send_head != NULL) {
--
2.7.4
^ permalink raw reply related
* Re: BPF: bug without effect in BPF_RSH case of adjust_scalar_min_max_vals()
From: Jann Horn @ 2017-12-04 20:26 UTC (permalink / raw)
To: Edward Cree
Cc: Network Development, kernel list, Alexei Starovoitov,
Daniel Borkmann
In-Reply-To: <CAG48ez1Fz_N+jA5=Nv-TDaVFcqqhE3K3hR8XG6SFpsmoeyhsuw@mail.gmail.com>
On Mon, Dec 4, 2017 at 6:03 PM, Jann Horn <jannh@google.com> wrote:
> As far as I can tell, commit b03c9f9fdc37 ("bpf/verifier: track signed
> and unsigned min/max values") introduced the following effectless bug
> in the BPF_RSH case of adjust_scalar_min_max_vals() (unless that's
> intentional):
[...]
> =======================
> BPF_LD_MAP_FD(BPF_REG_ARG1, mapfd),
>
> BPF_MOV64_REG(BPF_REG_TMP, BPF_REG_FP),
> BPF_ALU64_IMM(BPF_ADD, BPF_REG_TMP, -4), // allocate 4 bytes stack
> BPF_MOV32_IMM(BPF_REG_ARG2, 1),
> BPF_STX_MEM(BPF_W, BPF_REG_TMP, BPF_REG_ARG2, 0),
> BPF_MOV64_REG(BPF_REG_ARG2, BPF_REG_TMP),
> BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
> BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 2),
> BPF_MOV64_REG(BPF_REG_0, 0), // prepare exit
> BPF_EXIT_INSN(), // exit
> BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_0, 0),
>
> BPF_ALU64_IMM(BPF_AND, BPF_REG_3, 0xf),
> BPF_MOV64_IMM(BPF_REG_1, -42),
> BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_3),
> BPF_MOV64_IMM(BPF_REG_2, 2),
> BPF_ALU64_REG(BPF_RSH, BPF_REG_1, BPF_REG_2),
> BPF_EXIT_INSN()
> =======================
For using the eBPF bytecode in selftests:
Signed-off-by: Jann Horn <jannh@google.com>
^ permalink raw reply
* Re: BPF: bug without effect in BPF_RSH case of adjust_scalar_min_max_vals()
From: Edward Cree @ 2017-12-04 20:23 UTC (permalink / raw)
To: Jann Horn, Alexei Starovoitov, Daniel Borkmann
Cc: Network Development, kernel list
In-Reply-To: <CAG48ez1Fz_N+jA5=Nv-TDaVFcqqhE3K3hR8XG6SFpsmoeyhsuw@mail.gmail.com>
On 04/12/17 17:03, Jann Horn wrote:
> As far as I can tell, commit b03c9f9fdc37 ("bpf/verifier: track signed
> and unsigned min/max values") introduced the following effectless bug
> in the BPF_RSH case of adjust_scalar_min_max_vals() (unless that's
> intentional):
>
> `dst_reg->smax_value` is only updated in the case where
> `dst_reg->smin_value < 0` and `umin_val == 0`. This is obviously
> harmless if `dst_reg->smax_value >= 0`, but if `dst_reg->smax_value <
> 0`, this will temporarily result in a state where the signed upper
> bound of `dst_reg` is lower than the signed lower bound (which will be
> set to 0). I don't think this should ever happen.
Yep, I think you're right that there's a bug there; but I'm not sure it's
harmless in the dst_reg->smax_value >= 0 case either. Consider: if
dst_reg->smin_value < 0 and dst_reg->smax_value >= 0, then -1 is a
possible value; and ((u64)-1) >> 1 == S64_MAX. So in that case we have
to set dst_reg->smax_value = ((u64)-1) >> umin_val (so long as umin_val
isn't 0, which is the other branch).
If dst_reg->smax_value < 0, then we should set dst_reg->smax_value =
((u64)dst_reg->smax_value) >> umin_val, again excepting the case of
umin_val == 0.
Thanks for spotting this!
I'll rustle up a patch tomorrow, unless you beat me to it. Can I have an
SOB for your BPF bytecode, so I can incorporate it into selftests?
-Ed
^ permalink raw reply
* Re: [PATCH] ethtool: Add ETHTOOL_RESET support via --reset command
From: Scott Branden @ 2017-12-04 20:16 UTC (permalink / raw)
To: Gal Pressman, John W. Linville
Cc: BCM Kernel Feedback, Steve Lin, Michael Chan, netdev
In-Reply-To: <d240f7d8-9655-df14-2c85-d8aa8db80319@mellanox.com>
Hi Gal,
I do not understand you're comment - questions inline
On 17-12-03 12:07 AM, Gal Pressman wrote:
> On 30-Nov-17 21:24, Scott Branden wrote:
>> Add ETHTOOL_RESET support via --reset command.
>>
>> ie. ethtool --reset DEVNAME <flagname(s)>
>>
>> flagnames currently match the ETH_RESET_xxx names:
>> mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all
>>
>> Alternatively, you can specific component bitfield directly using
>> ethtool --reset DEVNAME flags %x
>>
>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>> ---
>> ethtool.8.in | 55 ++++++++++++++++++++++++++++++++++++++++++++-
>> ethtool.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 127 insertions(+), 1 deletion(-)
>>
>> diff --git a/ethtool.8.in b/ethtool.8.in
>> index 6ad3065..925cfe3 100644
>> --- a/ethtool.8.in
>> +++ b/ethtool.8.in
>> @@ -355,6 +355,20 @@ ethtool \- query or control network driver and hardware settings
>> .B ethtool \-\-get\-phy\-tunable
>> .I devname
>> .RB [ downshift ]
>> +.HP
>> +.B ethtool \-\-reset
>> +.I devname
>> +.BN flags
>> +.B [mgmt]
>> +.B [irq]
>> +.B [dma]
>> +.B [filter]
>> +.B [offload]
>> +.B [mac]
>> +.B [phy]
>> +.B [ram]
>> +.B [dedicated]
>> +.B [all]
>> .
> Nit:
> Usually, the brackets formatting is different than the keyword inside them.
Could you please explain what the formatting should be then? Any of the
options listed are possible components that can be selected. At least
one must be chosen. Should the brackets be removed then?
>
> Gal
^ permalink raw reply
* ethtool-copy.h out of sync with linux header file?
From: Scott Branden @ 2017-12-04 20:07 UTC (permalink / raw)
To: linville, paul.greenwalt; +Cc: netdev
Hi Paul/John,
I see a commit "ethtool: Add DMA Coalescing support" was added to
ethtool-copy.h yet I cannot find the change in the ethtool.h in the
linux kernel.
As such, I am unable to commit a change to ethtool-copy.h to sync it
with net-next. If I sync then ethtool will fail to compile.
Please advise.
Thanks,
Scott
^ permalink raw reply
* Re: [PATCH 1/2] net: sh_eth: use correct "struct device" when calling DMA mapping functions
From: Sergei Shtylyov @ 2017-12-04 20:05 UTC (permalink / raw)
To: Thomas Petazzoni, David S. Miller, Niklas Söderlund,
Geert Uytterhoeven, Simon Horman
Cc: netdev, linux-renesas-soc
In-Reply-To: <20171204133327.3505-2-thomas.petazzoni@free-electrons.com>
On 12/04/2017 04:33 PM, Thomas Petazzoni wrote:
> There are two types of "struct device": the one representing the
> physical device on its physical bus (platform, SPI, PCI, etc.), and
> the one representing the logical device in its device class (net,
> etc.).
>
> The DMA mapping API expects to receive as argument a "struct device"
> representing the physical device, as the "struct device" contains
> information about the bus that the DMA API needs.
>
> However, the sh_eth driver mistakenly uses the "struct device"
> representing the logical device (embedded in "struct net_device")
> rather than the "struct device" representing the physical device on
> its bus.
>
> This commit fixes that by adjusting all calls to the DMA mapping API.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
> drivers/net/ethernet/renesas/sh_eth.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 7e060aa9fbed..91e918e654fe 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -1149,7 +1149,8 @@ static int sh_eth_tx_free(struct net_device *ndev, bool sent_only)
> entry, le32_to_cpu(txdesc->status));
> /* Free the original skb. */
> if (mdp->tx_skbuff[entry]) {
> - dma_unmap_single(&ndev->dev, le32_to_cpu(txdesc->addr),
> + dma_unmap_single(&mdp->pdev->dev,
Using 'ndev->dev.parent' (as in ravb) also should work... not sure which
is better (although I'm seeing very strange things in the ravb driver built
with AArch654 gcc 4.8.5).
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW
From: Michael Chan @ 2017-12-04 19:52 UTC (permalink / raw)
To: Alexander Duyck; +Cc: David Miller, Netdev, Ariel Elior, everest-linux-l2
In-Reply-To: <CAKgT0Uc9Nb3jaiz1VRt6bUpjAZGKwUM9xrjvoJUbEAyCKpGXVA@mail.gmail.com>
On Mon, Dec 4, 2017 at 10:43 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>>>> Introduce NETIF_F_GRO_HW feature flag for NICs that support hardware
>>>> GRO. With this flag, we can now independently turn on or off hardware
>>>> GRO when GRO is on. Hardware GRO guarantees that packets can be
>>>> re-segmented by TSO/GSO to reconstruct the original packet stream.
>>>>
>>>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
>>>> Cc: everest-linux-l2@cavium.com
>>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>>>
>>> Do we really need yet another feature bit for this? We already have
>>> LRO and GRO and now we have to add something that isn't quite either
>>> one?
>>
>> I think so, to be consistent with TSO/GSO on the transmit side. On
>> the receive side, we have LRO/GRO_HW/GRO. There is difference between
>> LRO/GRO_HW that we need to distinguish between the 2.
>
> I don't really see the difference. Your GRO_HW likely doens't do all
> of the stuff GRO can do. Neither does LRO. Both occur in the hardware
> normally. It would make sense to reuse the LRO flag for this instead
> of coming up with a new feature flag that makes things confusing by
> saying you are doing a software offload in hardware.
I agree it is a little confusing, but LRO + private flag is just as
confusing in my opinion.
>
> I view LRO as a subset of what GRO can handle, that is performed in
> hardware.
I don't view LRO as a subset of GRO. LRO loses information that
cannot be reconstructed back once an LRO frame is aggregated. LRO is
just different from GRO. GRO_HW must provide enough information to
reconstruct the original frames.
> From the stack perspective the only thing that really
> matters is that the frames can be segmented back into what they were
> before they were assembled. That is why I think it would be better to
> add a flag indicating that the LRO is reversible instead of adding yet
> another feature bit that the user has to toggle. That way if at some
> point in the future an issue is found where your "GRO in hardware"
> feature has a bug that isn't reversible it is just a matter of
> clearing the privage flag bit and the mechanisms already in place for
> dealing with assembly and routing can take over.
NETIF_F_GRO_HW is a flag that depends on NETIF_F_GRO. In some ways,
it is similar to a private flag that depends on NETIF_F_LRO. But I
think a standard flag is better.
>
>>>
>>> I think I would rather have something like a netdev private flag that
>>> says LRO assembled frames are routable and just have this all run over
>>> the LRO flag with a test for the private flag to avoid triggering the
>>> LRO disable in the case of the flag being present. Really this is just
>>> a clean LRO implementation anyway so maybe we should just go that
>>> route where LRO is the hardware offload and GRO is the generic
>>> software implementation of that offload. That way when GRO gets some
>>> new feature that your hardware doesn't support we don't have to argue
>>> about the differences since LRO is meant to be a limited
>>> implementation anyway due to the nature of it being in hardware.
>>
>> Private flag will work. But a standard feature flag is better since
>> there are multiple drivers supporting this. A standard way to turn
>> this on/off is a better user experience. It's also consistent with
>> TSO/GSO on the transmit side.
>
> I agree, and that is why I would prefer to see this use the LRO flag.
> It is the flag that is normally used to indicating Rx coalescing in
> hardware. Coming up with a custom feature flag for a form of LRO that
> your hardware does doesn't make much sense to me. Otherwise I might as
> well go modify ixgbe and rename the LRO it does to GRO_HW since I can
> make it do most of what you are doing here.
Again, there is enough difference between LRO and hardware GRO that it
makes sense to add a new flag. Functionally, a private flag will work
too, but a standard flag makes more intuitive sense to me and to
users.
Yeah, the idea is that any vendor can use GRO_HW. Today, there are 3
drivers supporting it. I'm sure there will be other drivers
supporting this in the future. For something that is supported by
multiple vendors, that's another reason to use a standard flag.
>
>>>
>>> To me it just seems like this is an attempt to use the GRO name as a
>>> marketing term and I really don't like the feel of it.
>>>
>>
>> I disagree with this. It's more than a marketing term.
>
> Not really. It is a subset of GRO offload in hardware. In my mind that
> is just LRO. I say subset since odds are you don't support all of the
> same protocols and tunnels that GRO in software does.
Of course, hardware has some limitations, such as the number of TCP
connections it can aggregate, etc. But again, it is different from
LRO.
^ permalink raw reply
* RE: [Patch net v2] tipc: fix a null pointer deref on error path
From: Jon Maloy @ 2017-12-04 19:49 UTC (permalink / raw)
To: Cong Wang, David Miller
Cc: Linux Kernel Network Developers,
tipc-discussion@lists.sourceforge.net, Ying Xue
In-Reply-To: <CAM_iQpXX587pzZSHttJorv5k9wLq5ywG9hKKDQ5sqMSDtZJErQ@mail.gmail.com>
> -----Original Message-----
> From: Cong Wang [mailto:xiyou.wangcong@gmail.com]
> Sent: Monday, December 04, 2017 14:41
> To: David Miller <davem@davemloft.net>
> Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; tipc-
> discussion@lists.sourceforge.net; Jon Maloy <jon.maloy@ericsson.com>;
> Ying Xue <ying.xue@windriver.com>
> Subject: Re: [Patch net v2] tipc: fix a null pointer deref on error path
>
> On Mon, Dec 4, 2017 at 11:23 AM, Cong Wang <xiyou.wangcong@gmail.com>
> wrote:
> > On Mon, Dec 4, 2017 at 10:57 AM, David Miller <davem@davemloft.net>
> wrote:
> >>
> >> It looks like tipc_accept_from_sock() has a similar problem? The
> >> tipc_close_conn() will get invoked indirectly from the sock_release()
> >> path right?
> >
> > Not sure, the sock_release() in tipc_accept_from_sock() is for
> > kernel_accept(), not for tipc_alloc_conn(). Or maybe it is hiding deep
> > in the call chain that I miss?
>
> I see:
>
> tipc_release() -> tipc_sk_leave() -> tipc_group_delete()
> -> tipc_topsrv_kern_unsubscr() -> tipc_close_conn()
>
> Seems on this path we do need to skip NULL too.
You are right. The right solution is to just call conn_put() twice here.
I already have a patch ready for this, but it is part of a series that needs more review.
I should probably post it separately...
///jon
^ permalink raw reply
* Re: [bisected] x86 boot still broken on -rc2
From: Jakub Kicinski @ 2017-12-04 19:48 UTC (permalink / raw)
To: Prarit Bhargava
Cc: linux-kernel, netdev@vger.kernel.org, Thomas Gleixner,
Clark Williams
In-Reply-To: <20171204164521.17870-1-prarit@redhat.com>
On Mon, 4 Dec 2017 11:45:21 -0500, Prarit Bhargava wrote:
> On 12/04/2017 08:13 AM, Prarit Bhargava wrote:
> > x86: Booting SMP configuration:
> > .... node #0, CPUs: #1 #2 #3 #4
> > .... node #1, CPUs: #5 #6 #7 #8 #9
> > .... node #0, CPUs: #10 #11 #12 #13 #14
> > .... node #1, CPUs: #15 #16 #17 #18 #19
> > smp: Brought up 2 nodes, 20 CPUs
> > smpboot: Max logical packages: 1
> >
> > which means that the calculation of logical packages is wrong because
> >
> > ncpus = cpu_data(0).booted_cores * smp_num_siblings;
> > ncpus = 10 * 2;
> > ncpus = 20;
> >
> > smp_num_siblings is defined as "The number of threads in a core" which
> > should be 1 if HT/SMT is disabled.
> >
> > It looks like my patch has exposed a bug in the
> > smp_num_siblings calculation. I'm still debugging ...
>
> The bug is that smp_num_siblings has been incorrectly calculated as the
> *maximum* number of threads in a core, and not the actual number of threads in
> a core on systems which have a CPUID level greater than 0xb. (see
> arch/x86/kernel/cpu/topology.c:59)
>
> That will take some time to investigate and come up with a proper solution and
> fix. In the meantime, the patch below will fix the problem in the short-term.
> I've tested the patch using SMT enabled, SMT disabled, maxcpus=1 and nr_cpus=1.
Thanks Prarit, the work around does the job! Indeed, I have SMT
disabled.
^ permalink raw reply
* Re: [Patch net v2] tipc: fix a null pointer deref on error path
From: David Miller @ 2017-12-04 19:48 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, tipc-discussion, jon.maloy, ying.xue
In-Reply-To: <CAM_iQpV20zj=ZcQQy8TckQr2CJgvj=YDxgOsXnHj4pACOiD--g@mail.gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 4 Dec 2017 11:23:13 -0800
> On Mon, Dec 4, 2017 at 10:57 AM, David Miller <davem@davemloft.net> wrote:
>> From: Cong Wang <xiyou.wangcong@gmail.com>
>> Date: Mon, 4 Dec 2017 10:31:43 -0800
>>
>>> In tipc_topsrv_kern_subscr() when s->tipc_conn_new() fails
>>> we call tipc_close_conn() to clean up, but in this case
>>> calling conn_put() is just enough.
>>>
>>> This fixes the folllowing crash:
>> ...
>>> Fixes: 14c04493cb77 ("tipc: add ability to order and receive topology events in driver")
>>> Reported-by: syzbot <syzkaller@googlegroups.com>
>>> Cc: Jon Maloy <jon.maloy@ericsson.com>
>>> Cc: Ying Xue <ying.xue@windriver.com>
>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>> ...
>>> @@ -511,7 +511,7 @@ bool tipc_topsrv_kern_subscr(struct net *net, u32 port, u32 type,
>>> s = con->server;
>>> scbr = s->tipc_conn_new(*conid);
>>> if (!scbr) {
>>> - tipc_close_conn(con);
>>> + conn_put(con);
>>> return false;
>>> }
>>>
>>> --
>>> 2.13.0
>>>
>>
>> It looks like tipc_accept_from_sock() has a similar problem? The
>> tipc_close_conn() will get invoked indirectly from the sock_release()
>> path right?
>
> Not sure, the sock_release() in tipc_accept_from_sock() is for
> kernel_accept(), not for tipc_alloc_conn(). Or maybe it is hiding
> deep in the call chain that I miss?
I guess I'm trying to figure out where 'newcon' is released when the
call to s->tipc_conn_new() on it fails.
It looks similar to the situation you are fixing here, which is why I
am mentioning it.
^ permalink raw reply
* RE: [Patch net v2] tipc: fix a null pointer deref on error path
From: Jon Maloy @ 2017-12-04 19:44 UTC (permalink / raw)
To: David Miller, xiyou.wangcong@gmail.com
Cc: netdev@vger.kernel.org, tipc-discussion@lists.sourceforge.net,
Ying Xue
In-Reply-To: <20171204.135716.731762293431821544.davem@davemloft.net>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of David Miller
> Sent: Monday, December 04, 2017 13:57
> To: xiyou.wangcong@gmail.com
> Cc: netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net; Jon
> Maloy <jon.maloy@ericsson.com>; Ying Xue <ying.xue@windriver.com>
> Subject: Re: [Patch net v2] tipc: fix a null pointer deref on error path
>
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Mon, 4 Dec 2017 10:31:43 -0800
>
> > In tipc_topsrv_kern_subscr() when s->tipc_conn_new() fails we call
> > tipc_close_conn() to clean up, but in this case calling conn_put() is
> > just enough.
> >
> > This fixes the folllowing crash:
> ...
> > Fixes: 14c04493cb77 ("tipc: add ability to order and receive topology
> > events in driver")
> > Reported-by: syzbot <syzkaller@googlegroups.com>
> > Cc: Jon Maloy <jon.maloy@ericsson.com>
> > Cc: Ying Xue <ying.xue@windriver.com>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ...
> > @@ -511,7 +511,7 @@ bool tipc_topsrv_kern_subscr(struct net *net, u32
> port, u32 type,
> > s = con->server;
> > scbr = s->tipc_conn_new(*conid);
> > if (!scbr) {
> > - tipc_close_conn(con);
> > + conn_put(con);
> > return false;
> > }
> >
> > --
> > 2.13.0
> >
>
> It looks like tipc_accept_from_sock() has a similar problem? The
> tipc_close_conn() will get invoked indirectly from the sock_release()
> path right?
No, it doesn't. There will be a 'leaked' conn instance which will remain in the reference table until it is flushed during module removal.
We'll fix this in a separate patch. Cong's fix is correct.
///jon
^ permalink raw reply
* Re: [PATCH iproute2] ip route: broken logic when using default word and family not specified
From: Alexander Zubkov @ 2017-12-04 19:41 UTC (permalink / raw)
To: netdev
In-Reply-To: <CABXn0zd5eRTyWnNtpmMpHY=dWxtzRSXKtqw3wov=W2hH8Hs0og@mail.gmail.com>
Hello everybody,
I will be glad to hear a piece of feedback on this proposal.
On Sat, Nov 18, 2017 at 5:56 PM, Alexander Zubkov <zubkov318@gmail.com> wrote:
> I also opened earlier a ticket in bugzilla:
> https://bugzilla.kernel.org/show_bug.cgi?id=197899
> And Stephen Hemminger had couple of comments there which I want to argue:
>
>> $ ip route list default
>> Means list all routes in any address family (ie same as any)
>> but
>>
>> $ ip route list 0/0
>> Means list all routes for IPv4 default route.
>
> This is not correct, because first command do not show routes in any
> address family. Now it do so only when table 0 is specified, otherwise
> only IPv4 routes are showed. Here is the code from iproute.c:
>
> if (do_ipv6 == AF_UNSPEC && filter.tb)
> do_ipv6 = AF_INET;
>
>> It probably is worth a man page warning, but changing semantics
>> that have existed for many years is more likely to break some existing user.
>
> Yes, backward compatibility is a reason. But as I remember, that
> sematics already have changed earlier. Probably it was showing IPv4
> and IPv6 routes together without family specified - I do not remember
> exactly. And I have doubts that such feature could be lied on
> reliably.
> I as a end user would prefer to make the behaviour more consistent and
> without such excetptions. But of course there may be other opinions.
>
> On Sat, Nov 18, 2017 at 2:12 PM, Alexander Zubkov <zubkov318@gmail.com> wrote:
>> Hello,
>>
>> I have found odd behaviour when using "ip route list" (and other bound
>> commands) with prefix "default".
>>
>> When family not specified, its value is completely ignored and "ip
>> route list default" shows all inet4 prefixes. Same do "ip route list
>> exact default" and "ip route list match default". Examples are at the
>> end of the message.
>>
>> When family is specified, the behaviour changes and default works as
>> expected (=0.0.0.0/0 for inet4 and =::/0 for inet6). The above
>> commands all shows only default prefix in the output and only "root
>> default" shows all prefixes.
>>
>> I tried to dig into the code and found that when default is using with
>> unspecified family - the resulting structures filter.[mr]dst will
>> actually become all-zeroes as in the case when nothing is specified.
>>
>> I propose to change this in such a way (see attached patch). When
>> default prefix is parsed, the flag PREFIXLEN_SPECIFIED is attached to
>> it too, like for prefixes with "/<masklen>". It seems logical to me,
>> because "/0" is really implied by "default" and even directly set up
>> in the code:
>>
>> dst->bitlen = 0;
>>
>> Then during filtering there is additional logic for unspecified family
>> and specified prefix.
>>
>> With this patch ip route list commands shown above are working as
>> expected. And it also works with unspecified table when routes are
>> printed from different families.
>>
>> Examples after applying the patch:
>>
>> # ./ip route list
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> 192.168.1.0/24 via 192.168.0.3 dev eth0
>> # ./ip -6 route list
>> fe80::/64 dev eth0 proto kernel metric 256 pref medium
>> fe80:1::/64 via fe80::3 dev eth0 metric 1024 pref medium
>> default via fe80::2 dev eth0 metric 1024 pref medium
>> # ./ip route list default
>> default via 192.168.0.2 dev eth0
>> # ./ip route list exact default
>> default via 192.168.0.2 dev eth0
>> # ./ip route list match default
>> default via 192.168.0.2 dev eth0
>> # ./ip route list root default
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> 192.168.1.0/24 via 192.168.0.3 dev eth0
>> # ./ip route list default table all
>> default via 192.168.0.2 dev eth0
>> unreachable default dev lo proto kernel metric 4294967295 error -101 pref medium
>> default via fe80::2 dev eth0 metric 1024 pref medium
>> unreachable default dev lo proto kernel metric 4294967295 error -101 pref medium
>> unreachable default dev lo proto kernel metric 4294967295 error -101 pref medium
>> # ./ip route list root default table all
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> ...
>> ff00::/8 dev eth0 table local metric 256 pref medium
>> unreachable default dev lo proto kernel metric 4294967295 error -101 pref medium
>>
>> And before patch:
>>
>> # ip route list default
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> 192.168.1.0/24 via 192.168.0.3 dev eth0
>> # ip route list match default
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> 192.168.1.0/24 via 192.168.0.3 dev eth0
>> # ip route list exact default
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> 192.168.1.0/24 via 192.168.0.3 dev eth0
>> # ip -4 route list exact default
>> default via 192.168.0.2 dev eth0
>> # ip -6 route list exact default
>> default via fe80::2 dev eth0 metric 1024
>> # ip route list exact default table all
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> ...
>> local fe80::3c09:19ff:feee:9866 dev lo table local proto none metric 0
>> ff00::/8 dev eth0 table local metric 256
>> unreachable default dev lo table unspec proto kernel metric
>> 4294967295 error -101
^ permalink raw reply
* Re: [Patch net v2] tipc: fix a null pointer deref on error path
From: Cong Wang @ 2017-12-04 19:41 UTC (permalink / raw)
To: David Miller
Cc: Linux Kernel Network Developers, tipc-discussion, Jon Maloy,
Ying Xue
In-Reply-To: <CAM_iQpV20zj=ZcQQy8TckQr2CJgvj=YDxgOsXnHj4pACOiD--g@mail.gmail.com>
On Mon, Dec 4, 2017 at 11:23 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Dec 4, 2017 at 10:57 AM, David Miller <davem@davemloft.net> wrote:
>>
>> It looks like tipc_accept_from_sock() has a similar problem? The
>> tipc_close_conn() will get invoked indirectly from the sock_release()
>> path right?
>
> Not sure, the sock_release() in tipc_accept_from_sock() is for
> kernel_accept(), not for tipc_alloc_conn(). Or maybe it is hiding
> deep in the call chain that I miss?
I see:
tipc_release() -> tipc_sk_leave() -> tipc_group_delete()
-> tipc_topsrv_kern_unsubscr() -> tipc_close_conn()
Seems on this path we do need to skip NULL too.
^ permalink raw reply
* Re: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op
From: Shannon Nelson @ 2017-12-04 19:40 UTC (permalink / raw)
To: Yossi Kuperman, Aviv Heller, Steffen Klassert
Cc: Herbert Xu, Boris Pismenny, Yevgeny Kliteynik,
netdev@vger.kernel.org
In-Reply-To: <HE1PR05MB3210E26356A6402734E777EFC43F0@HE1PR05MB3210.eurprd05.prod.outlook.com>
On 12/3/2017 2:16 PM, Yossi Kuperman wrote:
>> -----Original Message-----
>> From: Shannon Nelson [mailto:shannon.nelson@oracle.com]
>> Sent: Sunday, December 3, 2017 12:11 AM
>> To: Aviv Heller <avivh@mellanox.com>; Steffen Klassert
>> <steffen.klassert@secunet.com>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>; Boris Pismenny
>> <borisp@mellanox.com>; Yossi Kuperman <yossiku@mellanox.com>;
>> Yevgeny Kliteynik <kliteyn@mellanox.com>; netdev@vger.kernel.org
>> Subject: Re: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op
>>
>> On 12/1/2017 11:47 AM, Shannon Nelson wrote:
>>> On 11/28/2017 9:55 AM, avivh@mellanox.com wrote:
>>>> From: Aviv Heller <avivh@mellanox.com>
>>>>
>>>> Adding the state to the offload device prior to replay init in
>>>> xfrm_state_construct() will result in NULL dereference if a matching
>>>> ESP packet is received in between.
>>>>
>>>> In order to inhibit driver offload logic from processing the state's
>>>> packets prior to the xfrm_state object being completely initialized
>>>> and added to the SADBs, a new activate() operation was added to
>>>> inform the driver the aforementioned conditions have been met.
>>>
>>> Are there also conditions where you would want to temporarily
>>> deactivate, or pause, the incoming driver offload, followed then by
>>> another activate?
>>>
>
> Nope.
>
>>> sln
>>
>> Instead of setting up a half-ready state that needs the activate() operation to
>> finish, can we instead just move the xfrm_dev_state_add() call to after the
>> xfrm_init_replay()? Especially since this really only makes sense for the
>> inbound, and makes no sense for the outbound path.
>>
>
> It might solve the problem this time, but still the state is not fully initialized and it might break
> in the future. Adding an SA to hardware can fail, and if we were to place xfrm_dev_state_add()
> after the SA is fully initialized, we will be forced to roll-back. Let alone the possible things that could
> go wrong once an SA is fully active while the hardware is oblivious. The core issue here is that we
> can't construct the SA and configure the hardware atomically, hence the two steps. The first step is
> to ensure the hardware can be configured properly, if not abort before the SA is initialized, and the
> second step is to just "mark" the state as active. Please note that the we don't explicitly mark it, it
> is active just by its existence in the hash table, which we already have and maintain.
>
>
> Out of curiosity, what's wrong with adding yet another callback (not on the critical-path)?
The existing model for hardware offloads is a single call to the driver
to enable or disable - either the hardware does the offload or it
doesn't. The closest examples are probably mac filters and vlan tags,
as well as flow filters: in all three cases the stack hands the offload
request to the driver and expects the offload to start at that point.
There is no in-between step of "here's an offload, but don't do it yet".
In reference the comment elsewhere about being "symmetric with
xdo_dev_state_delete and xdo_dev_state_free", I'm not convinced that the
xdo_dev_state_free is necessary: if the driver was already asked to
delete the SA, why should it care about a free later?
The driver model should be kept simple. There's no reason that I can
see for the driver to need a two-step commit, either for the add or for
the delete. When the stack asks the driver to do an offload, it should
be ready for the offload to happen.
sln
>
> Thanks for your comments.
>
>> sln
>>
>>>
>>>>
>>>> Signed-off-by: Aviv Heller <avivh@mellanox.com>
>>>> Signed-off-by: Yossi Kuperman <yossiku@mellanox.com>
>>>> ---
>>>> v1 -> v2:
>>>> - Separate to state addition and then activation, instead
>>>> of relocating dev state addition call.
>>>> ---
>>>> include/linux/netdevice.h | 1 +
>>>> include/net/xfrm.h | 12 ++++++++++++
>>>> net/xfrm/xfrm_user.c | 5 +++++
>>>> 3 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>> index 2eaac7d..c6ca356 100644
>>>> --- a/include/linux/netdevice.h
>>>> +++ b/include/linux/netdevice.h
>>>> @@ -819,6 +819,7 @@ struct netdev_xdp {
>>>> #ifdef CONFIG_XFRM_OFFLOAD
>>>> struct xfrmdev_ops {
>>>> int (*xdo_dev_state_add) (struct xfrm_state *x);
>>>> + void (*xdo_dev_state_activate) (struct xfrm_state *x);
>>>> void (*xdo_dev_state_delete) (struct xfrm_state *x);
>>>> void (*xdo_dev_state_free) (struct xfrm_state *x);
>>>> bool (*xdo_dev_offload_ok) (struct sk_buff *skb, diff --git
>>>> a/include/net/xfrm.h b/include/net/xfrm.h index e015e16..324374e
>>>> 100644
>>>> --- a/include/net/xfrm.h
>>>> +++ b/include/net/xfrm.h
>>>> @@ -1877,6 +1877,14 @@ static inline bool xfrm_dst_offload_ok(struct
>>>> dst_entry *dst)
>>>> return false;
>>>> }
>>>> +static inline void xfrm_dev_state_activate(struct xfrm_state *x) {
>>>> + struct xfrm_state_offload *xso = &x->xso;
>>>> +
>>>> + if (xso->dev && xso->dev->xfrmdev_ops->xdo_dev_state_activate)
>>>> + xso->dev->xfrmdev_ops->xdo_dev_state_activate(x);
>>>> +}
>>>> +
>>>> static inline void xfrm_dev_state_delete(struct xfrm_state *x)
>>>> {
>>>> struct xfrm_state_offload *xso = &x->xso; @@ -1907,6 +1915,10
>>>> @@ static inline int xfrm_dev_state_add(struct net *net, struct
>>>> xfrm_state *x, stru
>>>> return 0;
>>>> }
>>>> +static inline void xfrm_dev_state_activate(struct xfrm_state *x) { }
>>>> +
>>>> static inline void xfrm_dev_state_delete(struct xfrm_state *x)
>>>> {
>>>> }
>>>> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index
>>>> e44a0fe..d06f579 100644
>>>> --- a/net/xfrm/xfrm_user.c
>>>> +++ b/net/xfrm/xfrm_user.c
>>>> @@ -662,6 +662,11 @@ static int xfrm_add_sa(struct sk_buff *skb,
>>>> struct nlmsghdr *nlh,
>>>> goto out;
>>>> }
>>>> + spin_lock_bh(&x->lock);
>>>> + if (x->km.state == XFRM_STATE_VALID)
>>>> + xfrm_dev_state_activate(x);
>>>> + spin_unlock_bh(&x->lock);
>>>> +
>>>> c.seq = nlh->nlmsg_seq;
>>>> c.portid = nlh->nlmsg_pid;
>>>> c.event = nlh->nlmsg_type;
>>>>
^ permalink raw reply
* Re: iproute2: make ip route list to search by metric too
From: Alexander Zubkov @ 2017-12-04 19:39 UTC (permalink / raw)
To: netdev
In-Reply-To: <CABXn0zeE=NDTfBxG+tjrQ2o2Wt=Ji9wHuVG0dFTDW=3D551X=g@mail.gmail.com>
Hello everybody,
Excuse me for probably being impatient. But may I have some feedback
on my proposal?
On Sat, Nov 18, 2017 at 11:11 AM, Alexander Zubkov <zubkov318@gmail.com> wrote:
> I think this version will be better. It uses metric mask (like for
> some other options) instead of simple yes/no flag.
>
> On Sat, Nov 18, 2017 at 2:44 AM, Alexander Zubkov <zubkov318@gmail.com> wrote:
>> Hello again,
>>
>> Things turned out to be not so hard. Please take a look at the attached patch.
>> I'm only not sure if RTA_PRIORITY is enough. Because the print_route
>> function prints "metric" also for some situations with RTA_METRICS,
>> which I haven't managed to understand.
>>
>> On Fri, Nov 17, 2017 at 1:40 AM, Alexander Zubkov <zubkov318@gmail.com> wrote:
>>> Hello all,
>>>
>>> Currently routes in the Linux routing table have these "key" fields:
>>> prefix, tos, table, metric (as I know). I.e. we cannot have two
>>> different routes with the same set of this fields. And "ip route list"
>>> command can be provided with all but one of those fields. We cannot
>>> pass metric to it and this is inconvenient. I ask if this behaviour
>>> can be changed by someone. We can even use "secondary" fields, for
>>> example type, dev or via, but not metric unfortunately.
>>> Sorry, I can not provide patches. I have written code long time ago. I
>>> tried to trace it, but as I see it parses arguments and fills some
>>> structures. And then my tries to understand failed.
>>> I opened the bug: https://bugzilla.kernel.org/show_bug.cgi?id=197897,
>>> but I was pointed out that this mailing list is a better place for
>>> this question.
>>>
>>> --
>>> Alexander Zubkov
^ permalink raw reply
* Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW
From: Alexander Duyck @ 2017-12-04 19:36 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, Michael Chan, Netdev, Ariel Elior, everest-linux-l2
In-Reply-To: <1512415584.19682.60.camel@gmail.com>
On Mon, Dec 4, 2017 at 11:26 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2017-12-04 at 13:59 -0500, David Miller wrote:
>> From: Alexander Duyck <alexander.duyck@gmail.com>
>> Date: Mon, 4 Dec 2017 10:43:58 -0800
>>
>> > On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadco
>> m.com> wrote:
>> >> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
>> >> <alexander.duyck@gmail.com> wrote:
>> >>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadc
>> om.com> wrote:
>> >>>> Introduce NETIF_F_GRO_HW feature flag for NICs that support
>> hardware
>> >>>> GRO. With this flag, we can now independently turn on or off
>> hardware
>> >>>> GRO when GRO is on. Hardware GRO guarantees that packets can be
>> >>>> re-segmented by TSO/GSO to reconstruct the original packet
>> stream.
>> >>>>
>> >>>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
>> >>>> Cc: everest-linux-l2@cavium.com
>> >>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>> >>>
>> >>> Do we really need yet another feature bit for this? We already
>> have
>> >>> LRO and GRO and now we have to add something that isn't quite
>> either
>> >>> one?
>> >>
>> >> I think so, to be consistent with TSO/GSO on the transmit side.
>> On
>> >> the receive side, we have LRO/GRO_HW/GRO. There is difference
>> between
>> >> LRO/GRO_HW that we need to distinguish between the 2.
>> >
>> > I don't really see the difference. Your GRO_HW likely doens't do
>> all
>> > of the stuff GRO can do. Neither does LRO. Both occur in the
>> hardware
>> > normally. It would make sense to reuse the LRO flag for this
>> instead
>> > of coming up with a new feature flag that makes things confusing by
>> > saying you are doing a software offload in hardware.
>> >
>> > I view LRO as a subset of what GRO can handle, that is performed in
>> > hardware. From the stack perspective the only thing that really
>> > matters is that the frames can be segmented back into what they
>> were
>> > before they were assembled. That is why I think it would be better
>> to
>> > add a flag indicating that the LRO is reversible instead of adding
>> yet
>> > another feature bit that the user has to toggle. That way if at
>> some
>> > point in the future an issue is found where your "GRO in hardware"
>> > feature has a bug that isn't reversible it is just a matter of
>> > clearing the privage flag bit and the mechanisms already in place
>> for
>> > dealing with assembly and routing can take over.
>>
>> I don't think they should use the LRO flag.
>>
>> If their HW GRO stream is fully reversible, which it is, then it's
>> not
>> LRO.
>>
>> LRO gets disabled when bridging or routing is enabled, and HW GRO
>> should not take this penalty.
>
> Also having separate flags means that one can decide to disable HW GRO
> and enable (linux) GRO if he wants to. Or the opposite.
>
> I definitely like this idea.
Yeah, I get that. More specifically it is the bit where you have to
disable the hardware GRO due to a bug and then that also prevents you
from using the software GRO. The reuse of the GRO flag to represent a
hardware offload in general was a poor decision.
I just don't get why these drivers have to support both LRO or GRO_HW
at the same time. It seems like it is just overkill. I would rather
see them just use the LRO feature to represent hardware aggregation of
frames and then let the stack know if LRO is reversible or not for a
given device.
^ permalink raw reply
* Re: [PATCH v2 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint
From: Marcelo Ricardo Leitner @ 2017-12-04 19:28 UTC (permalink / raw)
To: Yafang Shao
Cc: davem, kuznet, yoshfuji, rostedt, songliubraving, bgregg, netdev,
linux-kernel
In-Reply-To: <1512207401-3154-1-git-send-email-laoar.shao@gmail.com>
On Sat, Dec 02, 2017 at 09:36:41AM +0000, Yafang Shao wrote:
> The TCP/IP transition from TCP_LISTEN to TCP_SYN_RECV and some other
> transitions are not traced with tcp_set_state tracepoint.
>
> In order to trace the whole tcp lifespans, two helpers are introduced,
> void __tcp_set_state(struct sock *sk, int state)
> void __sk_state_store(struct sock *sk, int newstate)
>
> When do TCP/IP state transition, we should use these two helpers or use
> tcp_set_state() other than assigning a value to sk_state directly.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>
> ---
> v2: test
> ---
> include/net/tcp.h | 2 ++
> net/ipv4/inet_connection_sock.c | 6 +++---
> net/ipv4/inet_hashtables.c | 2 +-
> net/ipv4/tcp.c | 12 ++++++++++++
> 4 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 85ea578..4f2d015 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1247,6 +1247,8 @@ static inline bool tcp_checksum_complete(struct sk_buff *skb)
> "Close Wait","Last ACK","Listen","Closing"
> };
> #endif
> +void __sk_state_store(struct sock *sk, int newstate);
> +void __tcp_set_state(struct sock *sk, int state);
> void tcp_set_state(struct sock *sk, int state);
>
> void tcp_done(struct sock *sk);
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 4ca46dc..f3967f1 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -783,7 +783,7 @@ struct sock *inet_csk_clone_lock(const struct sock *sk,
> if (newsk) {
> struct inet_connection_sock *newicsk = inet_csk(newsk);
>
> - newsk->sk_state = TCP_SYN_RECV;
> + __tcp_set_state(newsk, TCP_SYN_RECV);
> newicsk->icsk_bind_hash = NULL;
>
> inet_sk(newsk)->inet_dport = inet_rsk(req)->ir_rmt_port;
> @@ -877,7 +877,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
> * It is OK, because this socket enters to hash table only
> * after validation is complete.
> */
> - sk_state_store(sk, TCP_LISTEN);
> + __sk_state_store(sk, TCP_LISTEN);
> if (!sk->sk_prot->get_port(sk, inet->inet_num)) {
> inet->inet_sport = htons(inet->inet_num);
>
> @@ -888,7 +888,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
> return 0;
> }
>
> - sk->sk_state = TCP_CLOSE;
> + __tcp_set_state(sk, TCP_CLOSE);
> return err;
> }
> EXPORT_SYMBOL_GPL(inet_csk_listen_start);
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index e7d15fb..72c15b6 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -430,7 +430,7 @@ bool inet_ehash_nolisten(struct sock *sk, struct sock *osk)
> sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
> } else {
> percpu_counter_inc(sk->sk_prot->orphan_count);
> - sk->sk_state = TCP_CLOSE;
> + __tcp_set_state(sk, TCP_CLOSE);
> sock_set_flag(sk, SOCK_DEAD);
> inet_csk_destroy_sock(sk);
> }
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index bf97317..2bc7e04 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2036,6 +2036,18 @@ int tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int nonblock,
> }
> EXPORT_SYMBOL(tcp_recvmsg);
>
> +void __sk_state_store(struct sock *sk, int newstate)
> +{
> + trace_tcp_set_state(sk, sk->sk_state, newstate);
> + sk_state_store(sk, newstate);
This sounds counter-intuitive, to have a __func() to call func(). It's
usually the other way around.
There is only 1 call to sk_state_store in the stack, what about
inverting these __ ?
I guess you applied the same standard as to the one below, but it's a
different case.
> +}
> +
> +void __tcp_set_state(struct sock *sk, int state)
> +{
> + trace_tcp_set_state(sk, sk->sk_state, state);
> + sk->sk_state = state;
> +}
> +
> void tcp_set_state(struct sock *sk, int state)
> {
> int oldstate = sk->sk_state;
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW
From: Eric Dumazet @ 2017-12-04 19:26 UTC (permalink / raw)
To: David Miller, alexander.duyck
Cc: michael.chan, netdev, Ariel.Elior, everest-linux-l2
In-Reply-To: <20171204.135929.1654731726316874482.davem@davemloft.net>
On Mon, 2017-12-04 at 13:59 -0500, David Miller wrote:
> From: Alexander Duyck <alexander.duyck@gmail.com>
> Date: Mon, 4 Dec 2017 10:43:58 -0800
>
> > On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadco
> m.com> wrote:
> >> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
> >> <alexander.duyck@gmail.com> wrote:
> >>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadc
> om.com> wrote:
> >>>> Introduce NETIF_F_GRO_HW feature flag for NICs that support
> hardware
> >>>> GRO. With this flag, we can now independently turn on or off
> hardware
> >>>> GRO when GRO is on. Hardware GRO guarantees that packets can be
> >>>> re-segmented by TSO/GSO to reconstruct the original packet
> stream.
> >>>>
> >>>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
> >>>> Cc: everest-linux-l2@cavium.com
> >>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> >>>
> >>> Do we really need yet another feature bit for this? We already
> have
> >>> LRO and GRO and now we have to add something that isn't quite
> either
> >>> one?
> >>
> >> I think so, to be consistent with TSO/GSO on the transmit side.
> On
> >> the receive side, we have LRO/GRO_HW/GRO. There is difference
> between
> >> LRO/GRO_HW that we need to distinguish between the 2.
> >
> > I don't really see the difference. Your GRO_HW likely doens't do
> all
> > of the stuff GRO can do. Neither does LRO. Both occur in the
> hardware
> > normally. It would make sense to reuse the LRO flag for this
> instead
> > of coming up with a new feature flag that makes things confusing by
> > saying you are doing a software offload in hardware.
> >
> > I view LRO as a subset of what GRO can handle, that is performed in
> > hardware. From the stack perspective the only thing that really
> > matters is that the frames can be segmented back into what they
> were
> > before they were assembled. That is why I think it would be better
> to
> > add a flag indicating that the LRO is reversible instead of adding
> yet
> > another feature bit that the user has to toggle. That way if at
> some
> > point in the future an issue is found where your "GRO in hardware"
> > feature has a bug that isn't reversible it is just a matter of
> > clearing the privage flag bit and the mechanisms already in place
> for
> > dealing with assembly and routing can take over.
>
> I don't think they should use the LRO flag.
>
> If their HW GRO stream is fully reversible, which it is, then it's
> not
> LRO.
>
> LRO gets disabled when bridging or routing is enabled, and HW GRO
> should not take this penalty.
Also having separate flags means that one can decide to disable HW GRO
and enable (linux) GRO if he wants to. Or the opposite.
I definitely like this idea.
^ permalink raw reply
* Re: [PATCH] net: sh_eth: do not advertise Gigabit capabilities when not available
From: Andrew Lunn @ 2017-12-04 19:25 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Thomas Petazzoni, David S. Miller, Niklas Söderlund,
Geert Uytterhoeven, Simon Horman, netdev, linux-renesas-soc
In-Reply-To: <018f1129-826b-ecd7-9b71-b720706aa930@cogentembedded.com>
> >diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> >index db72d13cebb9..0074c5998481 100644
> >--- a/drivers/net/ethernet/renesas/sh_eth.c
> >+++ b/drivers/net/ethernet/renesas/sh_eth.c
> >@@ -1892,6 +1892,11 @@ static int sh_eth_phy_init(struct net_device *ndev)
> > return PTR_ERR(phydev);
> > }
> >+ /* mask with MAC supported features */
> >+ if (mdp->cd->register_type != SH_ETH_REG_GIGABIT)
> >+ phydev->supported &= PHY_BASIC_FEATURES;
> >+ phydev->advertising = phydev->supported;
> >+
>
> Um, looking at this again, I think we have phy_set_max_speed() for that
> kind of stuff now...
Ah, yes. I forgot about that.
Thomas, can you make a v2?
Andrew
^ permalink raw reply
* Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW
From: Alexander Duyck @ 2017-12-04 19:24 UTC (permalink / raw)
To: David Miller; +Cc: Michael Chan, Netdev, Ariel Elior, everest-linux-l2
In-Reply-To: <20171204.135929.1654731726316874482.davem@davemloft.net>
On Mon, Dec 4, 2017 at 10:59 AM, David Miller <davem@davemloft.net> wrote:
> From: Alexander Duyck <alexander.duyck@gmail.com>
> Date: Mon, 4 Dec 2017 10:43:58 -0800
>
>> On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>>> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>>>>> Introduce NETIF_F_GRO_HW feature flag for NICs that support hardware
>>>>> GRO. With this flag, we can now independently turn on or off hardware
>>>>> GRO when GRO is on. Hardware GRO guarantees that packets can be
>>>>> re-segmented by TSO/GSO to reconstruct the original packet stream.
>>>>>
>>>>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
>>>>> Cc: everest-linux-l2@cavium.com
>>>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>>>>
>>>> Do we really need yet another feature bit for this? We already have
>>>> LRO and GRO and now we have to add something that isn't quite either
>>>> one?
>>>
>>> I think so, to be consistent with TSO/GSO on the transmit side. On
>>> the receive side, we have LRO/GRO_HW/GRO. There is difference between
>>> LRO/GRO_HW that we need to distinguish between the 2.
>>
>> I don't really see the difference. Your GRO_HW likely doens't do all
>> of the stuff GRO can do. Neither does LRO. Both occur in the hardware
>> normally. It would make sense to reuse the LRO flag for this instead
>> of coming up with a new feature flag that makes things confusing by
>> saying you are doing a software offload in hardware.
>>
>> I view LRO as a subset of what GRO can handle, that is performed in
>> hardware. From the stack perspective the only thing that really
>> matters is that the frames can be segmented back into what they were
>> before they were assembled. That is why I think it would be better to
>> add a flag indicating that the LRO is reversible instead of adding yet
>> another feature bit that the user has to toggle. That way if at some
>> point in the future an issue is found where your "GRO in hardware"
>> feature has a bug that isn't reversible it is just a matter of
>> clearing the privage flag bit and the mechanisms already in place for
>> dealing with assembly and routing can take over.
>
> I don't think they should use the LRO flag.
>
> If their HW GRO stream is fully reversible, which it is, then it's not
> LRO.
I get all that. I was just hoping that we could phase out the old LRO
over time and push it more towards a GRO friendly implementation. It
is going to be annoying when a marketing person gets a hold of the
term because what you are going to end up with is people pushing for
LRO implementations to be tweaked to work like GRO just so they can
use the feature flag.
> LRO gets disabled when bridging or routing is enabled, and HW GRO
> should not take this penalty.
That is why I suggested a private flag indicating that LRO on this
device is reversible. Basically if it is reversible then we don't need
to disable it when routing/bridging.
If we insist on using the name GRO for this I would prefer GRO_PARTIAL
instead of GRO_HW. It would help to keep things symmetric as we have
been saying since GSO_PARTIAL is a partial implementation of GSO in
the hardware and this would be a partial implementation of GRO in the
hardware as it would be missing some protocols and functionality.
In addition if anything can be done so that the hardware GRO
implementation doesn't prevent software GRO from aggregating things
even larger if possible that would be preferred.
Anyway that is just my $.02 on this.
- Alex
^ permalink raw reply
* Re: [Patch net v2] tipc: fix a null pointer deref on error path
From: Cong Wang @ 2017-12-04 19:23 UTC (permalink / raw)
To: David Miller
Cc: Linux Kernel Network Developers, tipc-discussion, Jon Maloy,
Ying Xue
In-Reply-To: <20171204.135716.731762293431821544.davem@davemloft.net>
On Mon, Dec 4, 2017 at 10:57 AM, David Miller <davem@davemloft.net> wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Mon, 4 Dec 2017 10:31:43 -0800
>
>> In tipc_topsrv_kern_subscr() when s->tipc_conn_new() fails
>> we call tipc_close_conn() to clean up, but in this case
>> calling conn_put() is just enough.
>>
>> This fixes the folllowing crash:
> ...
>> Fixes: 14c04493cb77 ("tipc: add ability to order and receive topology events in driver")
>> Reported-by: syzbot <syzkaller@googlegroups.com>
>> Cc: Jon Maloy <jon.maloy@ericsson.com>
>> Cc: Ying Xue <ying.xue@windriver.com>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ...
>> @@ -511,7 +511,7 @@ bool tipc_topsrv_kern_subscr(struct net *net, u32 port, u32 type,
>> s = con->server;
>> scbr = s->tipc_conn_new(*conid);
>> if (!scbr) {
>> - tipc_close_conn(con);
>> + conn_put(con);
>> return false;
>> }
>>
>> --
>> 2.13.0
>>
>
> It looks like tipc_accept_from_sock() has a similar problem? The
> tipc_close_conn() will get invoked indirectly from the sock_release()
> path right?
Not sure, the sock_release() in tipc_accept_from_sock() is for
kernel_accept(), not for tipc_alloc_conn(). Or maybe it is hiding
deep in the call chain that I miss?
^ permalink raw reply
* Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW
From: David Miller @ 2017-12-04 18:59 UTC (permalink / raw)
To: alexander.duyck; +Cc: michael.chan, netdev, Ariel.Elior, everest-linux-l2
In-Reply-To: <CAKgT0Uc9Nb3jaiz1VRt6bUpjAZGKwUM9xrjvoJUbEAyCKpGXVA@mail.gmail.com>
From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Mon, 4 Dec 2017 10:43:58 -0800
> On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>>>> Introduce NETIF_F_GRO_HW feature flag for NICs that support hardware
>>>> GRO. With this flag, we can now independently turn on or off hardware
>>>> GRO when GRO is on. Hardware GRO guarantees that packets can be
>>>> re-segmented by TSO/GSO to reconstruct the original packet stream.
>>>>
>>>> Cc: Ariel Elior <Ariel.Elior@cavium.com>
>>>> Cc: everest-linux-l2@cavium.com
>>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>>>
>>> Do we really need yet another feature bit for this? We already have
>>> LRO and GRO and now we have to add something that isn't quite either
>>> one?
>>
>> I think so, to be consistent with TSO/GSO on the transmit side. On
>> the receive side, we have LRO/GRO_HW/GRO. There is difference between
>> LRO/GRO_HW that we need to distinguish between the 2.
>
> I don't really see the difference. Your GRO_HW likely doens't do all
> of the stuff GRO can do. Neither does LRO. Both occur in the hardware
> normally. It would make sense to reuse the LRO flag for this instead
> of coming up with a new feature flag that makes things confusing by
> saying you are doing a software offload in hardware.
>
> I view LRO as a subset of what GRO can handle, that is performed in
> hardware. From the stack perspective the only thing that really
> matters is that the frames can be segmented back into what they were
> before they were assembled. That is why I think it would be better to
> add a flag indicating that the LRO is reversible instead of adding yet
> another feature bit that the user has to toggle. That way if at some
> point in the future an issue is found where your "GRO in hardware"
> feature has a bug that isn't reversible it is just a matter of
> clearing the privage flag bit and the mechanisms already in place for
> dealing with assembly and routing can take over.
I don't think they should use the LRO flag.
If their HW GRO stream is fully reversible, which it is, then it's not
LRO.
LRO gets disabled when bridging or routing is enabled, and HW GRO
should not take this penalty.
^ 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