* [PATCH net-next 1/6] hv_netvsc: drop unused macros
From: Stephen Hemminger @ 2017-12-01 19:01 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20171201190149.22683-1-sthemmin@microsoft.com>
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/hyperv_net.h | 26 --------------------------
1 file changed, 26 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 88ddfb92122b..7226230561de 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -1425,32 +1425,6 @@ struct rndis_message {
(sizeof(msg) + (sizeof(struct rndis_message) - \
sizeof(union rndis_message_container)))
-/* get pointer to info buffer with message pointer */
-#define MESSAGE_TO_INFO_BUFFER(msg) \
- (((unsigned char *)(msg)) + msg->info_buf_offset)
-
-/* get pointer to status buffer with message pointer */
-#define MESSAGE_TO_STATUS_BUFFER(msg) \
- (((unsigned char *)(msg)) + msg->status_buf_offset)
-
-/* get pointer to OOBD buffer with message pointer */
-#define MESSAGE_TO_OOBD_BUFFER(msg) \
- (((unsigned char *)(msg)) + msg->oob_data_offset)
-
-/* get pointer to data buffer with message pointer */
-#define MESSAGE_TO_DATA_BUFFER(msg) \
- (((unsigned char *)(msg)) + msg->per_pkt_info_offset)
-
-/* get pointer to contained message from NDIS_MESSAGE pointer */
-#define RNDIS_MESSAGE_PTR_TO_MESSAGE_PTR(rndis_msg) \
- ((void *) &rndis_msg->msg)
-
-/* get pointer to contained message from NDIS_MESSAGE pointer */
-#define RNDIS_MESSAGE_RAW_PTR_TO_MESSAGE_PTR(rndis_msg) \
- ((void *) rndis_msg)
-
-
-
#define RNDIS_HEADER_SIZE (sizeof(struct rndis_message) - \
sizeof(union rndis_message_container))
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v2 net-next] net: netlink: Update attr validation to require exact length for some types
From: Jouni Malinen @ 2017-12-01 18:48 UTC (permalink / raw)
To: Jouni Malinen; +Cc: David Ahern, netdev
In-Reply-To: <CANe27jKPoF6GV6u0CjjYTUq0NrxFAS1ivEbhCtzd4ugEkL9=WA@mail.gmail.com>
Well.. This did not go well with gmail defaults and the mailing list..
Sending this with something safer and plaintext only version to get this
on the mailing list as well:
On Wed, Nov 8, 2017 at 7:59 AM, David Ahern <dsahern@gmail.com> wrote:
> Attributes using NLA_U* and NLA_S* (where * is 8, 16,32 and 64) are
> expected to be an exact length. Split these data types from
> nla_attr_minlen into nla_attr_len and update validate_nla to require
> the attribute to have exact length for them.
While I understand and support this change in general, I have to note that
this resulted in some unfortunate user space regressions that came apparent
when testing Wi-Fi with Linux 4.15-rc1. When a new nl80211 attribute was
added for controlling SMPS modes in 2014 the kernel contribution added this
with NLA_U8 policy while the user space contribution to hostapd used
NLA_PUT_U32. This has apparently been unnoticed until now since the first
byte contained the appropriate value on little endian devices (no one
testing this on big endian hosts?)..
I'll obviously fix the encoding of this attribute in hostapd, but it should
be noted that Linux 4.15 will result in significant functionality issues if
the kernel is updated without a user space fix going in first.
--
Jouni Malinen PGP id EFC895FA
^ permalink raw reply
* Re: [PATCH net] tcp/dccp: block bh before arming time_wait timer
From: Maciej Żenczykowski @ 2017-12-01 18:48 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <1512151616.19682.44.camel@gmail.com>
Acked-by: Maciej Żenczykowski <maze@google.com>
^ permalink raw reply
* Re: [PATCH 1/1] phy: Add 2.5G SGMII interface mode
From: Florian Fainelli @ 2017-12-01 18:44 UTC (permalink / raw)
To: Bhaskar Upadhaya, Russell King - ARM Linux, Andrew Lunn
Cc: netdev@vger.kernel.org, davem@davemloft.net,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <AM3PR04MB449A2C7FDF78431360961E48C390@AM3PR04MB449.eurprd04.prod.outlook.com>
On 12/01/2017 10:40 AM, Bhaskar Upadhaya wrote:
>
>> -----Original Message-----
>> From: Russell King - ARM Linux [mailto:linux@armlinux.org.uk]
>> Sent: Friday, December 01, 2017 12:23 AM
>> To: Andrew Lunn <andrew@lunn.ch>
>> Cc: Bhaskar Upadhaya <bhaskar.upadhaya@nxp.com>; netdev@vger.kernel.org;
>> davem@davemloft.net; linux-arm-kernel@lists.infradead.org
>> Subject: Re: [PATCH 1/1] phy: Add 2.5G SGMII interface mode
>>
>> On Thu, Nov 30, 2017 at 07:26:21PM +0100, Andrew Lunn wrote:
>>> On Thu, Nov 30, 2017 at 06:15:20PM +0000, Russell King - ARM Linux wrote:
>>>> On Thu, Nov 30, 2017 at 06:41:27PM +0100, Andrew Lunn wrote:
>>>>> SGMII supports passing auto-negotiation results from the PHY to
>>>>> the MAC. 1000BASE-X does not.
>>>>>
>>>>> SGMII supports the PHY running at 10, 100, and 1000 Mbps. But to
>>>>> support this, the MAC needs to replicate the bits 100, or 10 times
>>>>> when the PHY is running in 10 or 100Mbps mode.
>>>>>
>>>>> So with your 2.5G SGMII, you need to replicate the bits 250, 25,
>>>>> or
>>>>> 2.5 times if they PHY is running at lower speeds. This last one is
>>>>> interesting.
>>>>
>>>> That's not what I've read so far - but I don't know about the PHY in
>>>> this exact case because the docs are only available under NDA (which
>>>> makes it incredibly difficult to have this discussion.)
>>>>
>>>> However, from what I can ascertain from a Xilinx document, 2.5G is
>>>> 1G SGMII clocked 2.5x faster. When in 2.5G mode, the other modes
>>>> are unavailable.
>>>
>>> Hi Russell
>>>
>>> Thanks for looking into the details.
>>>
>>> So you need the PHY driver to see what it has negotiated, and when it
>>> calls the adjust_link callback, the MAC needs look at the
>>> phydev->interface and set the MAC to 2.5G SGMII or SGMII.
>>
>> Yes, it /looks/ that way from what I've read so far. I'd like to do a more
>> comprehensive look around at what various manufacturers are doing before
>> saying that definitively. Even better would be to find a specification for 2.5G
>> SGMII!
>>
>>> Same as the Marvell 10G PHY driver flips between
>>> PHY_INTERFACE_MODE_10GKR and PHY_INTERFACE_MODE_SGMII depending
>> on
>>> what it has negotiated.
>>
>> Having played extensively with the Marvell 88x3310 and thoroughly inspected its
>> registers and analysed its behaviour, I'd be hessitant to compare other PHYs with
>> it. The 3310 PHY seems to be multiple PHY blocks (from two vendors) in a single
>> package, and it dynamically switches in the appropriate hardware blocks
>> according to the negotiated link modes.
>>
>> However, given that 1G SGMII is clocked at a different rate from 2.5G SGMII,
>> both ends would need to be configured the same.
>>
>> For example, both mvneta and mvpp2.2 support 2.5G modes merely by
>> reconfiguring the comphy block, and all that does is increase the serdes clock.
>> That's used for 2500BASE-X (which I have had working locally through a devmem2
>> pokes on Clearfog and the older Marvell pp2x driver on Macchiatobin) but it's
>> more difficult to find 2.5G copper SFP modules to test with - which would
>> presumably talk 2.5G SGMII.
>
> So do we need to introduce a new interface type PHY_INTERFACE_MODE_2500SGMII or is there a way to deal with 2.5G SGMII ?
Well, because you are proposing this change, we were sort of hoping you
could explain the rationale, and point us to the 2.5 SGMII standard
document so we can understand what this is about....
--
Florian
^ permalink raw reply
* RE: [PATCH 1/1] phy: Add 2.5G SGMII interface mode
From: Bhaskar Upadhaya @ 2017-12-01 18:40 UTC (permalink / raw)
To: Russell King - ARM Linux, Andrew Lunn
Cc: netdev@vger.kernel.org, davem@davemloft.net,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20171130185300.GG10595@n2100.armlinux.org.uk>
>-----Original Message-----
>From: Russell King - ARM Linux [mailto:linux@armlinux.org.uk]
>Sent: Friday, December 01, 2017 12:23 AM
>To: Andrew Lunn <andrew@lunn.ch>
>Cc: Bhaskar Upadhaya <bhaskar.upadhaya@nxp.com>; netdev@vger.kernel.org;
>davem@davemloft.net; linux-arm-kernel@lists.infradead.org
>Subject: Re: [PATCH 1/1] phy: Add 2.5G SGMII interface mode
>
>On Thu, Nov 30, 2017 at 07:26:21PM +0100, Andrew Lunn wrote:
>> On Thu, Nov 30, 2017 at 06:15:20PM +0000, Russell King - ARM Linux wrote:
>> > On Thu, Nov 30, 2017 at 06:41:27PM +0100, Andrew Lunn wrote:
>> > > SGMII supports passing auto-negotiation results from the PHY to
>> > > the MAC. 1000BASE-X does not.
>> > >
>> > > SGMII supports the PHY running at 10, 100, and 1000 Mbps. But to
>> > > support this, the MAC needs to replicate the bits 100, or 10 times
>> > > when the PHY is running in 10 or 100Mbps mode.
>> > >
>> > > So with your 2.5G SGMII, you need to replicate the bits 250, 25,
>> > > or
>> > > 2.5 times if they PHY is running at lower speeds. This last one is
>> > > interesting.
>> >
>> > That's not what I've read so far - but I don't know about the PHY in
>> > this exact case because the docs are only available under NDA (which
>> > makes it incredibly difficult to have this discussion.)
>> >
>> > However, from what I can ascertain from a Xilinx document, 2.5G is
>> > 1G SGMII clocked 2.5x faster. When in 2.5G mode, the other modes
>> > are unavailable.
>>
>> Hi Russell
>>
>> Thanks for looking into the details.
>>
>> So you need the PHY driver to see what it has negotiated, and when it
>> calls the adjust_link callback, the MAC needs look at the
>> phydev->interface and set the MAC to 2.5G SGMII or SGMII.
>
>Yes, it /looks/ that way from what I've read so far. I'd like to do a more
>comprehensive look around at what various manufacturers are doing before
>saying that definitively. Even better would be to find a specification for 2.5G
>SGMII!
>
>> Same as the Marvell 10G PHY driver flips between
>> PHY_INTERFACE_MODE_10GKR and PHY_INTERFACE_MODE_SGMII depending
>on
>> what it has negotiated.
>
>Having played extensively with the Marvell 88x3310 and thoroughly inspected its
>registers and analysed its behaviour, I'd be hessitant to compare other PHYs with
>it. The 3310 PHY seems to be multiple PHY blocks (from two vendors) in a single
>package, and it dynamically switches in the appropriate hardware blocks
>according to the negotiated link modes.
>
>However, given that 1G SGMII is clocked at a different rate from 2.5G SGMII,
>both ends would need to be configured the same.
>
>For example, both mvneta and mvpp2.2 support 2.5G modes merely by
>reconfiguring the comphy block, and all that does is increase the serdes clock.
>That's used for 2500BASE-X (which I have had working locally through a devmem2
>pokes on Clearfog and the older Marvell pp2x driver on Macchiatobin) but it's
>more difficult to find 2.5G copper SFP modules to test with - which would
>presumably talk 2.5G SGMII.
So do we need to introduce a new interface type PHY_INTERFACE_MODE_2500SGMII or is there a way to deal with 2.5G SGMII ?
>
>--
>RMK's Patch system:
>https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.ar
>mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=02%7C01%7Cbhaskar.upadha
>ya%40nxp.com%7C6ed1aa95e2844f1f316008d538239cf6%7C686ea1d3bc2b4c6f
>a92cd99c5c301635%7C0%7C0%7C636476647969399947&sdata=z35Urdlslv7Cdq
>hmj6JUd48QyXrwjzrldIoFP9TEA6E%3D&reserved=0
>FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
>According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* Re: [PATCH 11/11] phylink: convert to fwnode
From: Florian Fainelli @ 2017-12-01 18:19 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Jonathan Corbet; +Cc: linux-doc, netdev
In-Reply-To: <E1eKiVJ-0004zs-5m@rmk-PC.armlinux.org.uk>
On 12/01/2017 02:25 AM, Russell King wrote:
> Convert phylink to fwnode, switching phylink_create() from taking a
> device_node to taking a fwnode_handle. This will allow other firmware
> systems to take advantage of sfp/phylink support.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH 10/11] sfp: convert to fwnode
From: Florian Fainelli @ 2017-12-01 18:18 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Jonathan Corbet; +Cc: linux-doc, netdev
In-Reply-To: <E1eKiVD-0004zl-NV@rmk-PC.armlinux.org.uk>
On 12/01/2017 02:25 AM, Russell King wrote:
> Convert sfp-bus to use fwnode rather than device_node internally, so
> we can support more than just device tree firmware.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH 09/11] sfp: fix sparse warning
From: Florian Fainelli @ 2017-12-01 18:17 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Jonathan Corbet; +Cc: linux-doc, netdev
In-Reply-To: <E1eKiV8-0004ze-Kx@rmk-PC.armlinux.org.uk>
On 12/01/2017 02:24 AM, Russell King wrote:
> drivers/net/phy/sfp-bus.c:298:13: warning: context imbalance in 'sfp_bus_release' - wrong count at exit
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* [PATCH net-next] bpf: Add access to snd_cwnd and others in sock_ops
From: Lawrence Brakmo @ 2017-12-01 18:15 UTC (permalink / raw)
To: netdev
Cc: Alexei Starovoitov, Daniel Borkmann, Kernel Team, Blake Matheny,
Vlad Dumitrescu
Adds read access to snd_cwnd and srtt_us fields of tcp_sock. Since these
fields are only valid if the socket associated with the sock_ops program
call is a full socket, the field is_fullsock is also added to the
bpf_sock_ops struct. If the socket is not a full socket, reading these
fields returns 0.
Note that in most cases it will not be necessary to check is_fullsock to
know if there is a full socket. The context of the call, as specified by
the 'op' field, can sometimes determine whether there is a full socket.
The struct bpf_sock_ops has the following fields added:
__u32 is_fullsock; /* Some TCP fields are only valid if
* there is a full socket. If not, the
* fields read as zero.
*/
__u32 snd_cwnd;
__u32 srtt_us; /* Averaged RTT << 3 in usecs */
There is a new macro, SOCK_OPS_GET_TCP32(NAME), to make it easier to add
read access to more 32 bit tcp_sock fields.
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
---
include/linux/filter.h | 1 +
include/net/tcp.h | 6 ++++--
include/uapi/linux/bpf.h | 6 ++++++
net/core/filter.c | 36 ++++++++++++++++++++++++++++++++++++
4 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 80b5b48..0062302 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -985,6 +985,7 @@ struct bpf_sock_ops_kern {
u32 reply;
u32 replylong[4];
};
+ u32 is_fullsock;
};
#endif /* __LINUX_FILTER_H__ */
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 4e09398..89a6560 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2012,10 +2012,12 @@ static inline int tcp_call_bpf(struct sock *sk, int op)
struct bpf_sock_ops_kern sock_ops;
int ret;
- if (sk_fullsock(sk))
+ memset(&sock_ops, 0, sizeof(sock_ops));
+ if (sk_fullsock(sk)) {
+ sock_ops.is_fullsock = 1;
sock_owned_by_me(sk);
+ }
- memset(&sock_ops, 0, sizeof(sock_ops));
sock_ops.sk = sk;
sock_ops.op = op;
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 4c223ab..80d62e8 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -941,6 +941,12 @@ struct bpf_sock_ops {
__u32 local_ip6[4]; /* Stored in network byte order */
__u32 remote_port; /* Stored in network byte order */
__u32 local_port; /* stored in host byte order */
+ __u32 is_fullsock; /* Some TCP fields are only valid if
+ * there is a full socket. If not, the
+ * fields read as zero.
+ */
+ __u32 snd_cwnd;
+ __u32 srtt_us; /* Averaged RTT << 3 in usecs */
};
/* List of known BPF sock_ops operators.
diff --git a/net/core/filter.c b/net/core/filter.c
index 6a85e67..bf31236 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4437,6 +4437,42 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
*insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->dst_reg,
offsetof(struct sock_common, skc_num));
break;
+
+ case offsetof(struct bpf_sock_ops, is_fullsock):
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(
+ struct bpf_sock_ops_kern,
+ is_fullsock),
+ si->dst_reg, si->src_reg,
+ offsetof(struct bpf_sock_ops_kern,
+ is_fullsock));
+ break;
+
+/* Helper macro for adding read access to tcp_sock fields. */
+#define SOCK_OPS_GET_TCP32(FIELD_NAME) \
+ do { \
+ BUILD_BUG_ON(FIELD_SIZEOF(struct tcp_sock, FIELD_NAME) != 4); \
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF( \
+ struct bpf_sock_ops_kern, \
+ is_fullsock), \
+ si->dst_reg, si->src_reg, \
+ offsetof(struct bpf_sock_ops_kern, \
+ is_fullsock)); \
+ *insn++ = BPF_JMP_IMM(BPF_JEQ, si->dst_reg, 0, 2); \
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF( \
+ struct bpf_sock_ops_kern, sk),\
+ si->dst_reg, si->src_reg, \
+ offsetof(struct bpf_sock_ops_kern, sk));\
+ *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg, \
+ offsetof(struct tcp_sock, FIELD_NAME)); \
+ } while (0)
+
+ case offsetof(struct bpf_sock_ops, snd_cwnd):
+ SOCK_OPS_GET_TCP32(snd_cwnd);
+ break;
+
+ case offsetof(struct bpf_sock_ops, srtt_us):
+ SOCK_OPS_GET_TCP32(srtt_us);
+ break;
}
return insn - insn_buf;
}
--
2.9.5
^ permalink raw reply related
* Fw: [Bug 198055] New: Missing mechanism to find maximum datagram sizes (mandated by RFC 1122)
From: Stephen Hemminger @ 2017-12-01 18:07 UTC (permalink / raw)
To: netdev
Begin forwarded message:
Date: Fri, 01 Dec 2017 15:54:14 +0000
From: bugzilla-daemon@bugzilla.kernel.org
To: stephen@networkplumber.org
Subject: [Bug 198055] New: Missing mechanism to find maximum datagram sizes (mandated by RFC 1122)
https://bugzilla.kernel.org/show_bug.cgi?id=198055
Bug ID: 198055
Summary: Missing mechanism to find maximum datagram sizes
(mandated by RFC 1122)
Product: Networking
Version: 2.5
Kernel Version: 4.13.15-300.fc27.x86_64
Hardware: All
OS: Linux
Tree: Mainline
Status: NEW
Severity: high
Priority: P1
Component: Other
Assignee: stephen@networkplumber.org
Reporter: kwyut@byom.de
Regression: No
It's currently impossible to reliably implement an application layer protocol
with UDP sockets in Linux. Applications can retrieve the PMTU from IP_RECVERR
control messages but are unable to account for the variable size of IP and UDP
headers beyond hard-coding a large enough guess.
Relevant section of RFC 1122
4.1.4 UDP/APPLICATION LAYER INTERFACE
The application interface to UDP MUST provide the full services
of the IP/transport interface described in Section 3.4 of this
document. Thus, an application using UDP needs the functions
of the GET_SRCADDR(), GET_MAXSIZES(), ADVISE_DELIVPROB(), and
RECV_ICMP() calls described in Section 3.4. For example,
GET_MAXSIZES() can be used to learn the effective maximum UDP
maximum datagram size for a particular {interface,remote
host,TOS} triplet.
In particular, GET_MAXSIZES() appears to be missing.
--
You are receiving this mail because:
You are the assignee for the bug.
^ permalink raw reply
* [PATCH net] tcp/dccp: block bh before arming time_wait timer
From: Eric Dumazet @ 2017-12-01 18:06 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Maciej Żenczykowski
From: Eric Dumazet <edumazet@google.com>
Maciej Żenczykowski reported some panics in tcp_twsk_destructor()
that might be caused by the following bug.
timewait timer is pinned to the cpu, because we want to transition
timwewait refcount from 0 to 4 in one go, once everything has been
initialized.
At the time commit ed2e92394589 ("tcp/dccp: fix timewait races in timer
handling") was merged, TCP was always running from BH habdler.
After commit 5413d1babe8f ("net: do not block BH while processing
socket backlog") we definitely can run tcp_time_wait() from process
context.
We need to block BH in the critical section so that the pinned timer
has still its purpose.
This bug is more likely to happen under stress and when very small RTO
are used in datacenter flows.
Fixes: 5413d1babe8f ("net: do not block BH while processing socket backlog")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Maciej Żenczykowski <maze@google.com>
---
net/dccp/minisocks.c | 6 ++++++
net/ipv4/tcp_minisocks.c | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/net/dccp/minisocks.c b/net/dccp/minisocks.c
index abd07a443219853b022bef41cb072e90ff8f07f0..178bb9833311f83205317b07fe64cb2e45a9f734 100644
--- a/net/dccp/minisocks.c
+++ b/net/dccp/minisocks.c
@@ -57,10 +57,16 @@ void dccp_time_wait(struct sock *sk, int state, int timeo)
if (state == DCCP_TIME_WAIT)
timeo = DCCP_TIMEWAIT_LEN;
+ /* tw_timer is pinned, so we need to make sure BH are disabled
+ * in following section, otherwise timer handler could run before
+ * we complete the initialization.
+ */
+ local_bh_disable();
inet_twsk_schedule(tw, timeo);
/* Linkage updates. */
__inet_twsk_hashdance(tw, sk, &dccp_hashinfo);
inet_twsk_put(tw);
+ local_bh_enable();
} else {
/* Sorry, if we're out of memory, just CLOSE this
* socket up. We've got bigger problems than
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index e36eff0403f4e80c4f7291a70614f40125652133..b079b619b60ca577d5ef20a5065fce87acecd96c 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -310,10 +310,16 @@ void tcp_time_wait(struct sock *sk, int state, int timeo)
if (state == TCP_TIME_WAIT)
timeo = TCP_TIMEWAIT_LEN;
+ /* tw_timer is pinned, so we need to make sure BH are disabled
+ * in following section, otherwise timer handler could run before
+ * we complete the initialization.
+ */
+ local_bh_disable();
inet_twsk_schedule(tw, timeo);
/* Linkage updates. */
__inet_twsk_hashdance(tw, sk, &tcp_hashinfo);
inet_twsk_put(tw);
+ local_bh_enable();
} else {
/* Sorry, if we're out of memory, just CLOSE this
* socket up. We've got bigger problems than
^ permalink raw reply related
* Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()
From: Jiri Pirko @ 2017-12-01 17:56 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, Jiri Pirko, Jamal Hadi Salim
In-Reply-To: <20171130225335.6957-1-xiyou.wangcong@gmail.com>
Thu, Nov 30, 2017 at 11:53:32PM CET, xiyou.wangcong@gmail.com wrote:
>tcfm_dev always points to the correct netdev and we already
>hold a refcnt, so no need to use ifindex to lookup again.
>
>If we would support moving target netdev across netns, using
>pointer would be better than ifindex.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>---
> include/net/tc_act/tc_mirred.h | 1 -
> net/sched/act_mirred.c | 3 +--
> 2 files changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
>index 21d253c9a8c6..b2dbbfaefd22 100644
>--- a/include/net/tc_act/tc_mirred.h
>+++ b/include/net/tc_act/tc_mirred.h
>@@ -11,7 +11,6 @@ struct tcf_mirred {
> int tcfm_ifindex;
> bool tcfm_mac_header_xmit;
> struct net_device __rcu *tcfm_dev;
>- struct net *net;
> struct list_head tcfm_list;
> };
> #define to_mirred(a) ((struct tcf_mirred *)a)
>diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>index 8b3e59388480..fe6489f9c3cf 100644
>--- a/net/sched/act_mirred.c
>+++ b/net/sched/act_mirred.c
>@@ -140,7 +140,6 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> m->tcfm_eaction = parm->eaction;
> if (dev != NULL) {
> m->tcfm_ifindex = parm->ifindex;
>- m->net = net;
Isn't this here so user may specify a ifindex of netdev which is not yet
present on the system (not sure how much sense that would make though...)
> if (ret != ACT_P_CREATED)
> dev_put(rcu_dereference_protected(m->tcfm_dev, 1));
> dev_hold(dev);
>@@ -318,7 +317,7 @@ static struct net_device *tcf_mirred_get_dev(const struct tc_action *a)
> {
> struct tcf_mirred *m = to_mirred(a);
>
>- return __dev_get_by_index(m->net, m->tcfm_ifindex);
>+ return rtnl_dereference(m->tcfm_dev);
> }
>
> static struct tc_action_ops act_mirred_ops = {
>--
>2.13.0
>
^ permalink raw reply
* Re: [PATCH/RFC] Re: 'perf test BPF' failing, libbpf regression wrt "basic API for BPF obj name"
From: Arnaldo Carvalho de Melo @ 2017-12-01 17:51 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Martin KaFai Lau, Wang Nan, Daniel Borkmann, David S. Miller,
David Ahern, Ingo Molnar, Linux Kernel Mailing List, netdev
In-Reply-To: <312f7691-cb7a-5c2f-18c6-ab26cfaa26a6@fb.com>
Em Thu, Nov 30, 2017 at 01:51:15PM -0800, Alexei Starovoitov escreveu:
> On 11/30/17 11:00 AM, Arnaldo Carvalho de Melo wrote:
> > > Instead of sinking all future bpf_attr's backward compatibility
> > > requirements to sys_bpf, I would push it up to its own BPF_* command
> > > helper which has a better sense of its bpf_attr, i.e. push it up
> > > to bpf_create_map_node() and bpf_load_program_name() in this case.
> > Humm, we could try that approach, but the one in this patch seemed good
> > enough.
> >
> > And after all if the first syscall() invokation, with the latest kernel
> > and latest tooling will work, right?
>
> I agree with Martin and I also don't think it will work to push
> logic of all bpf commands into single sys_bpf syscall wrapper.
Sure, that was just a POC, I'll work on something that takes into
account what you guys pointed out.
> This logic will become more and more complex over time.
> Like this case really belongs in bpf_create_map() which is a wrapper
> on top of single BPF_CREATE_MAP command.
> Note it's the first time we're facing this 'new libbpf.a running on
> top of old kernel' issue and should be very careful adding such
> fallback code to the generic bpf library, since all the selftests/bpf/
> are using this lib and relying on excepted behavior.
Right, tools/perf/ uses it as well and relies on its continued
functioning.
> We don't want tests that want to test the latest kernel feature all of
> a sudden pass on old kernel that doesn't have it.
Sure, neither do I :-)
> To some degree perf and selftests/bpf needs are diverging here,
> so adding #ifdef to libbpf.a to match testcase expectations may be
> necessary.
But this is not just testcase expectations, the usecase is someone
wanting to use a newer tool, with perhaps some new features of interest
that don't depend on changes in the kernel, in an older kernel on a
system where updating it is not possible or desirable.
- Arnaldo
^ permalink raw reply
* Re: [PATCH net-next 3/4] inet: Add a 2nd listener hashtable (port+addr)
From: Martin KaFai Lau @ 2017-12-01 17:47 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David S . Miller, Eric Dumazet, Kernel Team
In-Reply-To: <1512149206.19682.37.camel@gmail.com>
On Fri, Dec 01, 2017 at 09:26:46AM -0800, Eric Dumazet wrote:
> On Thu, 2017-11-30 at 15:23 -0800, Martin KaFai Lau wrote:
> > The current listener hashtable is hashed by port only.
> > When a process is listening at many IP addresses with the same port
> > (e.g.
> > [IP1]:443, [IP2]:443... [IPN]:443), the inet[6]_lookup_listener()
> > performance is degraded to a link list. It is prone to syn attack.
> >
> >
>
> ...
>
> > diff --git a/include/net/inet_hashtables.h
> > b/include/net/inet_hashtables.h
> > index 4cce516c41ac..ebce55d694e7 100644
> > --- a/include/net/inet_hashtables.h
> > +++ b/include/net/inet_hashtables.h
> > @@ -152,8 +152,19 @@ struct inet_hashinfo {
> > */
> > struct inet_listen_hashbucket listening_hash[INET_LHT
> > ABLE_SIZE]
> > ____cacheline_aligned_in_smp
> > ;
> > + struct inet_listen_hashbucket *lhash2;
> > + unsigned int lhash2_mask;
> > };
> >
>
> Please move these two fields before listening_hash[], to avoid adding
> another cache line to "struct inet_hashinfo"
>
> ( We have a 16-byte hole there, so lets use it ;) )
Oops. Indeed. Thanks for pointing it out.
^ permalink raw reply
* Re: [EXT] Re: [PATCH net] net: phylink: fix link state on phy-connect
From: Russell King - ARM Linux @ 2017-12-01 17:47 UTC (permalink / raw)
To: Florian Fainelli
Cc: Grygorii Strashko, Yan Markman, Antoine Tenart, andrew@lunn.ch,
davem@davemloft.net, gregory.clement@free-electrons.com,
thomas.petazzoni@free-electrons.com,
miquel.raynal@free-electrons.com, Nadav Haklai, mw@semihalf.com,
Stefan Chulski, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <221420a4-9f56-373e-f5cd-0d2fcb02e5fb@gmail.com>
On Fri, Dec 01, 2017 at 09:36:42AM -0800, Florian Fainelli wrote:
> On 12/01/2017 09:24 AM, Russell King - ARM Linux wrote:
> > On Fri, Dec 01, 2017 at 11:07:22AM -0600, Grygorii Strashko wrote:
> >> Hi Russell,
> >>
> >> On 11/30/2017 07:28 AM, Russell King - ARM Linux wrote:
> >>> On Thu, Nov 30, 2017 at 10:10:18AM +0000, Russell King - ARM Linux wrote:
> >>>> On Thu, Nov 30, 2017 at 08:51:21AM +0000, Yan Markman wrote:
> >>>>> The phylink_stop is called before phylink_disconnect_phy
> >>>>> You could see in mvpp2.c:
> >>>>>
> >>>>> mvpp2_stop_dev() {
> >>>>> phylink_stop(port->phylink);
> >>>>> }
> >>>>>
> >>>>> mvpp2_stop() {
> >>>>> mvpp2_stop_dev(port);
> >>>>> phylink_disconnect_phy(port->phylink);
> >>>>> }
> >>>>>
> >>>>> .ndo_stop = mvpp2_stop,
> >>>>
> >>>> Sorry, I don't have this in mvpp2.c, so I have no visibility of what
> >>>> you're working with.
> >>>>
> >>>> What you have above looks correct, and I see no reason why the p21
> >>>> patch would not have resolved your issue. The p21 patch ensures
> >>>> that phylink_resolve() gets called and completes before phylink_stop()
> >>>> returns. In that case, phylink_resolve() will call the mac_link_down()
> >>>> method if the link is not already down. It will also print the "Link
> >>>> is Down" message.
> >>>>
> >>>> Florian has already tested this patch after encountering a similar
> >>>> issue, and has reported that it solves the problem for him. I've also
> >>>> tested it with mvneta, and the original mvpp2x driver on Macchiatobin.
> >>>>
> >>>> Maybe there's something different about mvpp2, but as I have no
> >>>> visibility of that driver and the modifications therein, I can't
> >>>> comment further other than stating that it works for three different
> >>>> implementations.
> >>>>
> >>>> Maybe you could try and work out what's going on with the p21 patch
> >>>> in your case?
> >>>
> >>> I think I now realise what's probably going on.
> >>>
> >>> If you call netif_carrier_off() before phylink_stop(), then phylink will
> >>> believe that the link is already down, and so it won't bother calling
> >>> mac_link_down() - it will believe that the link is already down.
> >>>
> >>> I'll update the documentation for phylink_stop() to spell out this
> >>> aspect.
> >>>
> >>
> >> There are pretty high number of net drivers which do call
> >> netif_carrier_off(dev);
> >> before
> >> phy_stop(dev->phydev);
> >> in .ndo_stop() callback.
> >>
> >> As per you comment this seems to be incorrect, so should such calls be
> >> removed?
> >
> > Well, I think the question that needs to be asked is this:
> >
> > Is calling netif_carrier_off() before phy_stop() safe?
> >
> > Well, reading the phylib code, this is the answer I've come to:
> >
> > Between phy_start() and phy_stop(), phylib is free to manage the
> > carrier state itself through the phylib state machine.
> >
> > This means if you call netif_carrier_off() prior to phy_stop(),
> > there is nothing preventing the phylib state machine from running,
> > and a co-incident poll of the PHY could notice that the link has
> > come up, and re-enable the carrier while your ndo_stop() method
> > is still running.
> >
> > So, my conclusion is that this practice is provably racy, though
> > it's probably not that easy to trigger the race (which is probably
> > why no one has reported the problem.)
> >
> > Given that it's racy, it's not something that I think phylink should
> > care about, and should "softly" discourage it. So, I'm happy with
> > what phylink is doing here, and I suggest fixing the drivers for
> > this race.
> >
> > In any case, it should result in less code in the drivers - since
> > the work you need to do when the link goes down is a subset of the
> > work you need to do when the network interface is taken down.
> >
>
> While I agree with all of what written before, in practice, calling
> netif_carrier_off() when using PHYLIB can cause inconsistent carrier
> states at most, but it would not be messing the state machine itself
> because PHYLIB does not make uses of netif_carrier_ok() to make any
> decisions as whether the link has dropped or not, it bases its
> information solely on phydev->link.
Indeed, but the point I'm making is that this sequence is very
possible with drivers that mess about by fiddling with stuff
before they call phy_stop():
CPU0 CPU1
netif_carrier_off()
mvpp2_egress_disable()
phy_state_machine()
(phydev->state = PHY_AN)
phy_link_up()
phy_link_change()
netif_carrier_on()
mvpp2_link_event()
mvpp2_egress_enable()
mvpp2_ingress_enable()
mvpp2_port_disable()
phy_stop(ndev->phydev)
At this point, egress has not been disabled as mvpp2_stop_dev() wants,
because the phylib state machine got in before it was stopped, called
the adjust link function which then had the effect of re-enabling the
egress.
If that doesn't matter, then what's the point of the
mvpp2_egress_disable() call in the mvpp2_stop_dev() path... either
it matters and the mvpp2_stop_dev() sequence is broken, or it doesn't
matter and some the work that mvpp2_stop_dev() is doing is unnecessary.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* Re: [PATCH v4 3/8] MIPS: Octeon: Add a global resource manager.
From: David Daney @ 2017-12-01 17:42 UTC (permalink / raw)
To: Philippe Ombredanne, Carlos Munoz
Cc: David Daney, linux-mips, ralf, netdev, David S. Miller,
Rob Herring, Mark Rutland, devel, Greg Kroah-Hartman, LKML,
Steven J. Hill,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andrew Lunn, Florian Fainelli, James Hogan
In-Reply-To: <CAOFm3uGhRTTrvygBd0dMdzWZQC5kFi8yXuWQsnhDvDLtW2z7aA@mail.gmail.com>
On 11/30/2017 11:53 PM, Philippe Ombredanne wrote:
> Carlos,
>
> On Thu, Nov 30, 2017 at 11:53 PM, James Hogan <james.hogan@mips.com> wrote:
>> On Tue, Nov 28, 2017 at 04:55:35PM -0800, David Daney wrote:
>>> From: Carlos Munoz <cmunoz@cavium.com>
>>>
>>> Add a global resource manager to manage tagged pointers within
>>> bootmem allocated memory. This is used by various functional
>>> blocks in the Octeon core like the FPA, Ethernet nexus, etc.
>>>
>>> Signed-off-by: Carlos Munoz <cmunoz@cavium.com>
>>> Signed-off-by: Steven J. Hill <Steven.Hill@cavium.com>
>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>> ---
>>> arch/mips/cavium-octeon/Makefile | 3 +-
>>> arch/mips/cavium-octeon/resource-mgr.c | 371 +++++++++++++++++++++++++++++++++
>>> arch/mips/include/asm/octeon/octeon.h | 18 ++
>>> 3 files changed, 391 insertions(+), 1 deletion(-)
>>> create mode 100644 arch/mips/cavium-octeon/resource-mgr.c
>>>
>>> diff --git a/arch/mips/cavium-octeon/Makefile b/arch/mips/cavium-octeon/Makefile
>>> index 7c02e542959a..0a299ab8719f 100644
>>> --- a/arch/mips/cavium-octeon/Makefile
>>> +++ b/arch/mips/cavium-octeon/Makefile
>>> @@ -9,7 +9,8 @@
>>> # Copyright (C) 2005-2009 Cavium Networks
>>> #
>>>
>>> -obj-y := cpu.o setup.o octeon-platform.o octeon-irq.o csrc-octeon.o
>>> +obj-y := cpu.o setup.o octeon-platform.o octeon-irq.o csrc-octeon.o \
>>> + resource-mgr.o
>>
>> Maybe put that on a separate line like below.
>>
>>> obj-y += dma-octeon.o
>>> obj-y += octeon-memcpy.o
>>> obj-y += executive/
>>> diff --git a/arch/mips/cavium-octeon/resource-mgr.c b/arch/mips/cavium-octeon/resource-mgr.c
>>> new file mode 100644
>>> index 000000000000..ca25fa953402
>>> --- /dev/null
>>> +++ b/arch/mips/cavium-octeon/resource-mgr.c
>>> @@ -0,0 +1,371 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Resource manager for Octeon.
>>> + *
>>> + * This file is subject to the terms and conditions of the GNU General Public
>>> + * License. See the file "COPYING" in the main directory of this archive
>>> + * for more details.
>>> + *
>>> + * Copyright (C) 2017 Cavium, Inc.
>>> + */
>
> Since you nicely included an SPDX id, you would not need the
> boilerplate anymore. e.g. these can go alright?
They may not be strictly speaking necessary, but I don't think they hurt
anything. Unless there is a requirement to strip out the license text,
we would stick with it as is.
>
>>> + * This file is subject to the terms and conditions of the GNU General Public
>>> + * License. See the file "COPYING" in the main directory of this archive
>>> + * for more details.
>
^ permalink raw reply
* Re: netfilter: xt_bpf: Fix XT_BPF_MODE_FD_PINNED mode of 'xt_bpf_info_v1'
From: Al Viro @ 2017-12-01 17:39 UTC (permalink / raw)
To: Kees Cook
Cc: Shmulik Ladkani, Willem de Bruijn, Daniel Borkmann,
Pablo Neira Ayuso, Linus Torvalds, David Miller, LKML,
Network Development, Christoph Hellwig, Thomas Garnier, Jann Horn
In-Reply-To: <20171201045439.GO21978@ZenIV.linux.org.uk>
On Fri, Dec 01, 2017 at 04:54:39AM +0000, Al Viro wrote:
> On Fri, Dec 01, 2017 at 03:48:59AM +0000, Al Viro wrote:
>
> > Something similar to get_prog_path_type() above might make for a usable
> > primitive, IMO...
>
> Incidentally, bpf_obj_get_user()/bpf_obj_do_get() should just use
> user_path(), rather than wanking with getname()+kern_path(pname->name)+putname().
> Note that kern_path() will do getname_kernel() to get struct pathname...
>
> Would cause problems for tracepoints in there, though. And that, BTW,
> is precisely why I don't want tracepoints in core VFS, TYVM - makes
> restructuring the code harder...
Egads... Contortions in bpf ->mknod() are really obnoxious.
First of all, it checks that ->d_fsdata is non-NULL and fails otherwise.
The only time ->d_fsdata gets non-NULL on that fs? In bpf_obj_do_pin(), this:
dentry->d_fsdata = raw;
ret = vfs_mknod(dir, dentry, mode, devt);
dentry->d_fsdata = NULL;
In other words, it's *not* going to work from normal mknod(2). Why go through
->mknod(), then, especially since it requires that kind of contortions to
pass the data in?
devt is 0:1 or 0:2 here. mode? Character or block device, right? Like hell -
it's a regular file. And devt is a cute way to pass a flag down into bpf_mkobj()
(aka. ->mknod()) through vfs_mknod(). No, it doesn't go into ->i_rdev...
And to make the things even more fun, the damn thing is passed to a couple
of Linux S&M hooks - security_path_mknod() and security_inode_mknod(). Oh, sorry -
three hooks. There's devcgroup_inode_mknod() as well, but that thing sees S_IFREG
in mode and buggers off quietly. Our esteemed sadomaso^Wsecurity community gets
to play, though. Without any way to see _what_ are we attaching to that place in
the bpf fs tree, but hey - it's security, it doesn't need to make sense...
What the hell? If you need a clean way to do something, why don't you describe
(on fsdevel, or in off-list mail to relevant people) what do you really want?
Sure, you can "work around" anything, but doesn't that level of perversion
strike you as a clear sign of something being not right?
For crying out loud, you are trying to pass a tagged pointer to one or another
kind of object into your own function. For that you
* use a field in a globally visible data structure as a temporary storage
for a pointer
* encode your tag (essentially a boolean) into a fucking _device_ _number_,
of all things, and shove it through, hoping that no LSM module gets weirded out by
non-zero device number combined with regular file for mode.
If that does not scream "wrong or missing primitive", I don't know what would.
You want something along the lines of "create a filesystem object at given
location, calling this function with this argument for actual object creation"?
Fair enough, but then let's add a primitive that would do just that.
And grepping around for similar sick tricks catches a slightly milder example -
mq_open(2) doesn't play with encoding stuff into dev_t, but otherwise it's very
similar and could also benefit from the same primitive.
How about something like this:
int vfs_mkobj(struct dentry *dentry, umode_t mode,
int (*f)(struct dentry *, umode_t, void *),
void *arg)
{
struct inode *dir = dentry->d_parent->d_inode;
int error = may_create(dir, dentry);
if (error)
return error;
mode &= S_IALLUGO;
mode |= S_IFREG;
error = security_inode_create(dir, dentry, mode);
if (error)
return error;
error = f(dentry, mode, arg);
if (!error)
fsnotify_create(dir, dentry);
return error;
}
exported by fs/namei.c, with your code doing
switch (type) {
case BPF_TYPE_PROG:
error = vfs_mkobj(path.dentry, mode, bpf_mkprog, raw);
break;
case BPF_TYPE_MAP:
error = vfs_mkobj(path.dentry, mode, bpf_mkmap, raw);
break;
default:
error = -EPERM;
}
instead that vfs_mknod() hack, with
static int bpf_mkprog(struct inode *dir, struct dentry *dentry,
umode_t mode, void *raw)
{
return bpf_mkobj_ops(dir, dentry, mode, raw, &bpf_prog_iops);
}
static int bpf_mkmap(struct inode *dir, struct dentry *dentry,
umode_t mode, void *raw)
{
return bpf_mkobj_ops(dir, dentry, mode, raw, &bpf_map_iops);
}
static int bpf_mkobj_ops(struct inode *dir, struct dentry *dentry,
umode_t mode, void *raw, struct inode_operations *iops)
{
struct inode *inode;
inode = bpf_get_inode(dir->i_sb, dir, mode);
if (IS_ERR(inode))
return PTR_ERR(inode);
inode->i_op = iops;
inode->i_private = raw;
bpf_dentry_finalize(dentry, inode, dir);
return 0;
}
And to hell with messing with dev_t, ->d_fsdata or having ->mknod() there at all...
Might want to replace security_path_mknod() with something saner, while we are
at it.
Objections?
PS: mqueue.c would also benefit from such primitive - do_create() there would
simply pass attr as callback's argument into vfs_mkobj(), with callback being
the guts of mqueue_create()...
^ permalink raw reply
* Re: [EXT] Re: [PATCH net] net: phylink: fix link state on phy-connect
From: Florian Fainelli @ 2017-12-01 17:36 UTC (permalink / raw)
To: Russell King - ARM Linux, Grygorii Strashko
Cc: Yan Markman, Antoine Tenart, andrew@lunn.ch, davem@davemloft.net,
gregory.clement@free-electrons.com,
thomas.petazzoni@free-electrons.com,
miquel.raynal@free-electrons.com, Nadav Haklai, mw@semihalf.com,
Stefan Chulski, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20171201172440.GK10595@n2100.armlinux.org.uk>
On 12/01/2017 09:24 AM, Russell King - ARM Linux wrote:
> On Fri, Dec 01, 2017 at 11:07:22AM -0600, Grygorii Strashko wrote:
>> Hi Russell,
>>
>> On 11/30/2017 07:28 AM, Russell King - ARM Linux wrote:
>>> On Thu, Nov 30, 2017 at 10:10:18AM +0000, Russell King - ARM Linux wrote:
>>>> On Thu, Nov 30, 2017 at 08:51:21AM +0000, Yan Markman wrote:
>>>>> The phylink_stop is called before phylink_disconnect_phy
>>>>> You could see in mvpp2.c:
>>>>>
>>>>> mvpp2_stop_dev() {
>>>>> phylink_stop(port->phylink);
>>>>> }
>>>>>
>>>>> mvpp2_stop() {
>>>>> mvpp2_stop_dev(port);
>>>>> phylink_disconnect_phy(port->phylink);
>>>>> }
>>>>>
>>>>> .ndo_stop = mvpp2_stop,
>>>>
>>>> Sorry, I don't have this in mvpp2.c, so I have no visibility of what
>>>> you're working with.
>>>>
>>>> What you have above looks correct, and I see no reason why the p21
>>>> patch would not have resolved your issue. The p21 patch ensures
>>>> that phylink_resolve() gets called and completes before phylink_stop()
>>>> returns. In that case, phylink_resolve() will call the mac_link_down()
>>>> method if the link is not already down. It will also print the "Link
>>>> is Down" message.
>>>>
>>>> Florian has already tested this patch after encountering a similar
>>>> issue, and has reported that it solves the problem for him. I've also
>>>> tested it with mvneta, and the original mvpp2x driver on Macchiatobin.
>>>>
>>>> Maybe there's something different about mvpp2, but as I have no
>>>> visibility of that driver and the modifications therein, I can't
>>>> comment further other than stating that it works for three different
>>>> implementations.
>>>>
>>>> Maybe you could try and work out what's going on with the p21 patch
>>>> in your case?
>>>
>>> I think I now realise what's probably going on.
>>>
>>> If you call netif_carrier_off() before phylink_stop(), then phylink will
>>> believe that the link is already down, and so it won't bother calling
>>> mac_link_down() - it will believe that the link is already down.
>>>
>>> I'll update the documentation for phylink_stop() to spell out this
>>> aspect.
>>>
>>
>> There are pretty high number of net drivers which do call
>> netif_carrier_off(dev);
>> before
>> phy_stop(dev->phydev);
>> in .ndo_stop() callback.
>>
>> As per you comment this seems to be incorrect, so should such calls be
>> removed?
>
> Well, I think the question that needs to be asked is this:
>
> Is calling netif_carrier_off() before phy_stop() safe?
>
> Well, reading the phylib code, this is the answer I've come to:
>
> Between phy_start() and phy_stop(), phylib is free to manage the
> carrier state itself through the phylib state machine.
>
> This means if you call netif_carrier_off() prior to phy_stop(),
> there is nothing preventing the phylib state machine from running,
> and a co-incident poll of the PHY could notice that the link has
> come up, and re-enable the carrier while your ndo_stop() method
> is still running.
>
> So, my conclusion is that this practice is provably racy, though
> it's probably not that easy to trigger the race (which is probably
> why no one has reported the problem.)
>
> Given that it's racy, it's not something that I think phylink should
> care about, and should "softly" discourage it. So, I'm happy with
> what phylink is doing here, and I suggest fixing the drivers for
> this race.
>
> In any case, it should result in less code in the drivers - since
> the work you need to do when the link goes down is a subset of the
> work you need to do when the network interface is taken down.
>
While I agree with all of what written before, in practice, calling
netif_carrier_off() when using PHYLIB can cause inconsistent carrier
states at most, but it would not be messing the state machine itself
because PHYLIB does not make uses of netif_carrier_ok() to make any
decisions as whether the link has dropped or not, it bases its
information solely on phydev->link.
This is not true with PHYLINK, which is why the problem was observed here.
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 4/4] tcp: Enable 2nd listener hashtable in TCP
From: Eric Dumazet @ 2017-12-01 17:29 UTC (permalink / raw)
To: Martin KaFai Lau, netdev; +Cc: David S . Miller, Eric Dumazet, Kernel Team
In-Reply-To: <20171130232327.561108-5-kafai@fb.com>
On Thu, 2017-11-30 at 15:23 -0800, Martin KaFai Lau wrote:
> Enable the second listener hashtable in TCP.
> The scale is the same as UDP which is one slot per 2MB.
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
> net/ipv4/tcp.c | 3 +++
> 1 file changed, 3 insertions(+)
>
Reviewed-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH 08/11] sfp: add documentation for kernel APIs
From: Florian Fainelli @ 2017-12-01 17:28 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Jonathan Corbet; +Cc: linux-doc, netdev
In-Reply-To: <E1eKiV3-0004zX-9f@rmk-PC.armlinux.org.uk>
On 12/01/2017 02:24 AM, Russell King wrote:
> Add kernel-doc documentation for sfp kernel APIs, and link it into the
> networking kapi documentation under "Network device support".
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH 07/11] phylink: add documentation for kernel APIs
From: Florian Fainelli @ 2017-12-01 17:28 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Jonathan Corbet; +Cc: linux-doc, netdev
In-Reply-To: <E1eKiUx-0004zQ-TN@rmk-PC.armlinux.org.uk>
On 12/01/2017 02:24 AM, Russell King wrote:
> Add kernel-doc documentation for phylink kernel APIs, and link it into
> the networking kapi documentation under "Network device support".
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 3/4] inet: Add a 2nd listener hashtable (port+addr)
From: Eric Dumazet @ 2017-12-01 17:26 UTC (permalink / raw)
To: Martin KaFai Lau, netdev; +Cc: David S . Miller, Eric Dumazet, Kernel Team
In-Reply-To: <20171130232327.561108-4-kafai@fb.com>
On Thu, 2017-11-30 at 15:23 -0800, Martin KaFai Lau wrote:
> The current listener hashtable is hashed by port only.
> When a process is listening at many IP addresses with the same port
> (e.g.
> [IP1]:443, [IP2]:443... [IPN]:443), the inet[6]_lookup_listener()
> performance is degraded to a link list. It is prone to syn attack.
>
>
...
> diff --git a/include/net/inet_hashtables.h
> b/include/net/inet_hashtables.h
> index 4cce516c41ac..ebce55d694e7 100644
> --- a/include/net/inet_hashtables.h
> +++ b/include/net/inet_hashtables.h
> @@ -152,8 +152,19 @@ struct inet_hashinfo {
> */
> struct inet_listen_hashbucket listening_hash[INET_LHT
> ABLE_SIZE]
> ____cacheline_aligned_in_smp
> ;
> + struct inet_listen_hashbucket *lhash2;
> + unsigned int lhash2_mask;
> };
>
Please move these two fields before listening_hash[], to avoid adding
another cache line to "struct inet_hashinfo"
( We have a 16-byte hole there, so lets use it ;) )
^ permalink raw reply
* Re: [EXT] Re: [PATCH net] net: phylink: fix link state on phy-connect
From: Russell King - ARM Linux @ 2017-12-01 17:24 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Yan Markman, Antoine Tenart, andrew@lunn.ch, f.fainelli@gmail.com,
davem@davemloft.net, gregory.clement@free-electrons.com,
thomas.petazzoni@free-electrons.com,
miquel.raynal@free-electrons.com, Nadav Haklai, mw@semihalf.com,
Stefan Chulski, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <51496048-7e11-e0aa-7c47-3c04eee70e3a@ti.com>
On Fri, Dec 01, 2017 at 11:07:22AM -0600, Grygorii Strashko wrote:
> Hi Russell,
>
> On 11/30/2017 07:28 AM, Russell King - ARM Linux wrote:
> > On Thu, Nov 30, 2017 at 10:10:18AM +0000, Russell King - ARM Linux wrote:
> >> On Thu, Nov 30, 2017 at 08:51:21AM +0000, Yan Markman wrote:
> >>> The phylink_stop is called before phylink_disconnect_phy
> >>> You could see in mvpp2.c:
> >>>
> >>> mvpp2_stop_dev() {
> >>> phylink_stop(port->phylink);
> >>> }
> >>>
> >>> mvpp2_stop() {
> >>> mvpp2_stop_dev(port);
> >>> phylink_disconnect_phy(port->phylink);
> >>> }
> >>>
> >>> .ndo_stop = mvpp2_stop,
> >>
> >> Sorry, I don't have this in mvpp2.c, so I have no visibility of what
> >> you're working with.
> >>
> >> What you have above looks correct, and I see no reason why the p21
> >> patch would not have resolved your issue. The p21 patch ensures
> >> that phylink_resolve() gets called and completes before phylink_stop()
> >> returns. In that case, phylink_resolve() will call the mac_link_down()
> >> method if the link is not already down. It will also print the "Link
> >> is Down" message.
> >>
> >> Florian has already tested this patch after encountering a similar
> >> issue, and has reported that it solves the problem for him. I've also
> >> tested it with mvneta, and the original mvpp2x driver on Macchiatobin.
> >>
> >> Maybe there's something different about mvpp2, but as I have no
> >> visibility of that driver and the modifications therein, I can't
> >> comment further other than stating that it works for three different
> >> implementations.
> >>
> >> Maybe you could try and work out what's going on with the p21 patch
> >> in your case?
> >
> > I think I now realise what's probably going on.
> >
> > If you call netif_carrier_off() before phylink_stop(), then phylink will
> > believe that the link is already down, and so it won't bother calling
> > mac_link_down() - it will believe that the link is already down.
> >
> > I'll update the documentation for phylink_stop() to spell out this
> > aspect.
> >
>
> There are pretty high number of net drivers which do call
> netif_carrier_off(dev);
> before
> phy_stop(dev->phydev);
> in .ndo_stop() callback.
>
> As per you comment this seems to be incorrect, so should such calls be
> removed?
Well, I think the question that needs to be asked is this:
Is calling netif_carrier_off() before phy_stop() safe?
Well, reading the phylib code, this is the answer I've come to:
Between phy_start() and phy_stop(), phylib is free to manage the
carrier state itself through the phylib state machine.
This means if you call netif_carrier_off() prior to phy_stop(),
there is nothing preventing the phylib state machine from running,
and a co-incident poll of the PHY could notice that the link has
come up, and re-enable the carrier while your ndo_stop() method
is still running.
So, my conclusion is that this practice is provably racy, though
it's probably not that easy to trigger the race (which is probably
why no one has reported the problem.)
Given that it's racy, it's not something that I think phylink should
care about, and should "softly" discourage it. So, I'm happy with
what phylink is doing here, and I suggest fixing the drivers for
this race.
In any case, it should result in less code in the drivers - since
the work you need to do when the link goes down is a subset of the
work you need to do when the network interface is taken down.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* Re: [PATCH 06/11] phylink: restart 802.3z negotiation when starting net device
From: Florian Fainelli @ 2017-12-01 17:24 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Jonathan Corbet; +Cc: linux-doc, netdev
In-Reply-To: <E1eKiUs-0004zB-P8@rmk-PC.armlinux.org.uk>
On 12/01/2017 02:24 AM, Russell King wrote:
> Restart 802.3z negotiation when the net device is brought up to ensure
> that the link partner has our current link modes.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH 05/11] phylink: remove phylink_init_eee()
From: Florian Fainelli @ 2017-12-01 17:23 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Jonathan Corbet; +Cc: linux-doc, netdev
In-Reply-To: <E1eKiUn-0004yw-KY@rmk-PC.armlinux.org.uk>
On 12/01/2017 02:24 AM, Russell King wrote:
> phylink_init_eee() serves no purpose, remove it.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ 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