* [RFC PATCH v1 3/5] tcp: PSH frames sent without timer involved
From: Natale Patriciello @ 2017-07-28 19:59 UTC (permalink / raw)
To: David S . Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy
Cc: netdev, Ahmed Said, Natale Patriciello, Francesco Zampognaro,
Cesare Roseti
In-Reply-To: <20170728195919.10099-1-natale.patriciello@gmail.com>
Segments flagged with 'PSH' should be sent as soon as possible,
ignoring the timing set by the congestion control (if any).
This patch avoids the waiting of 'PSH' segments in the TCP queue.
Signed-off-by: Natale Patriciello <natale.patriciello@gmail.com>
Tested-by: Ahmed Said <ahmed.said@uniroma2.it>
---
net/ipv4/tcp.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 40aca7803cf2..ebaedbf75b63 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -975,9 +975,9 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
if (forced_push(tp)) {
tcp_mark_push(tp, skb);
- __tcp_push_pending_frames(sk, mss_now, TCP_NAGLE_PUSH);
- } else if (skb == tcp_send_head(sk))
tcp_push_one(sk, mss_now);
+ } else if (skb == tcp_send_head(sk))
+ __tcp_push_pending_frames(sk, mss_now, TCP_NAGLE_PUSH);
continue;
wait_for_sndbuf:
@@ -1320,9 +1320,9 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
if (forced_push(tp)) {
tcp_mark_push(tp, skb);
- __tcp_push_pending_frames(sk, mss_now, TCP_NAGLE_PUSH);
- } else if (skb == tcp_send_head(sk))
tcp_push_one(sk, mss_now);
+ } else if (skb == tcp_send_head(sk))
+ __tcp_push_pending_frames(sk, mss_now, TCP_NAGLE_PUSH);
continue;
wait_for_sndbuf:
--
2.13.2
^ permalink raw reply related
* [RFC PATCH v1 2/5] tcp: Implemented the timing-based operations
From: Natale Patriciello @ 2017-07-28 19:59 UTC (permalink / raw)
To: David S . Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy
Cc: netdev, Ahmed Said, Natale Patriciello, Francesco Zampognaro,
Cesare Roseti
In-Reply-To: <20170728195919.10099-1-natale.patriciello@gmail.com>
Timing the TCP operation based on the timer returned by the congestion control.
If the congestion control does not implement the timing interface, the TCP
behaves as usual, by sending down segments as soon as it is possible. Otherwise,
it will wait until the timer expires (and so respecting the timing constraint
set by the congestion control).
Signed-off-by: Natale Patriciello <natale.patriciello@gmail.com>
Tested-by: Ahmed Said <ahmed.said@uniroma2.it>
---
include/linux/tcp.h | 3 +++
net/ipv4/tcp_ipv4.c | 2 ++
net/ipv4/tcp_output.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 58 insertions(+), 3 deletions(-)
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index b6d5adcee8fc..140bc20ec17e 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -369,6 +369,9 @@ struct tcp_sock {
*/
struct request_sock *fastopen_rsk;
u32 *saved_syn;
+
+/* TCP send timer */
+ struct timer_list send_timer;
};
enum tsq_enum {
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 5ab2aac5ca19..ef5fdba096e8 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1351,6 +1351,8 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
if (*own_req)
tcp_move_syn(newtp, req);
+ init_timer(&newtp->send_timer);
+
return newsk;
exit_overflow:
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 4858e190f6ac..357b9cd5019e 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2187,6 +2187,7 @@ static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
int push_one, gfp_t gfp)
{
struct tcp_sock *tp = tcp_sk(sk);
+ const struct tcp_congestion_ops *ca_ops;
struct sk_buff *skb;
unsigned int tso_segs, sent_pkts;
int cwnd_quota;
@@ -2194,6 +2195,7 @@ static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
bool is_cwnd_limited = false, is_rwnd_limited = false;
u32 max_segs;
+ ca_ops = inet_csk(sk)->icsk_ca_ops;
sent_pkts = 0;
if (!push_one) {
@@ -2292,8 +2294,16 @@ static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
tcp_schedule_loss_probe(sk);
is_cwnd_limited |= (tcp_packets_in_flight(tp) >= tp->snd_cwnd);
tcp_cwnd_validate(sk, is_cwnd_limited);
+
+ /* Duplicated because of tp->prr_out value */
+ if (ca_ops && ca_ops->segment_sent)
+ ca_ops->segment_sent(sk, sent_pkts);
return false;
}
+
+ if (ca_ops && ca_ops->segment_sent)
+ ca_ops->segment_sent(sk, 0);
+
return !tp->packets_out && tcp_send_head(sk);
}
@@ -2433,6 +2443,15 @@ void tcp_send_loss_probe(struct sock *sk)
tcp_rearm_rto(sk);
}
+static void __tcp_push_pending_frames_handler(unsigned long data)
+{
+ struct sock *sk = (struct sock *)data;
+
+ lock_sock(sk);
+ tcp_push_pending_frames(sk);
+ release_sock(sk);
+}
+
/* Push out any pending frames which were held back due to
* TCP_CORK or attempt at coalescing tiny packets.
* The socket must be locked by the caller.
@@ -2440,6 +2459,8 @@ void tcp_send_loss_probe(struct sock *sk)
void __tcp_push_pending_frames(struct sock *sk, unsigned int cur_mss,
int nonagle)
{
+ struct tcp_sock *tp = tcp_sk(sk);
+
/* If we are closed, the bytes will have to remain here.
* In time closedown will finish, we empty the write queue and
* all will be happy.
@@ -2447,9 +2468,38 @@ void __tcp_push_pending_frames(struct sock *sk, unsigned int cur_mss,
if (unlikely(sk->sk_state == TCP_CLOSE))
return;
- if (tcp_write_xmit(sk, cur_mss, nonagle, 0,
- sk_gfp_mask(sk, GFP_ATOMIC)))
- tcp_check_probe_timer(sk);
+ if (timer_pending(&tp->send_timer) == 0) {
+ /* Timer is not running, push data out */
+ int ret;
+ const struct tcp_congestion_ops *ca_ops;
+
+ ca_ops = inet_csk(sk)->icsk_ca_ops;
+
+ if (ca_ops && ca_ops->send_timer_expired)
+ ca_ops->send_timer_expired(sk);
+
+ if (tcp_write_xmit(sk, cur_mss, nonagle, 0, sk_gfp_mask(sk, GFP_ATOMIC)))
+ tcp_check_probe_timer(sk);
+
+ /* And now let's init the timer only if we have data */
+ if (tcp_send_head(sk)) {
+ if (ca_ops && ca_ops->get_send_timer_exp_time) {
+ unsigned long expiration;
+
+ setup_timer(&tp->send_timer,
+ __tcp_push_pending_frames_handler,
+ (unsigned long)sk);
+ expiration = ca_ops->get_send_timer_exp_time(sk);
+ ret = mod_timer(&tp->send_timer,
+ jiffies + expiration);
+ BUG_ON(ret != 0);
+ }
+ } else {
+ del_timer(&tp->send_timer);
+ if (ca_ops && ca_ops->no_data_to_transmit)
+ ca_ops->no_data_to_transmit(sk);
+ }
+ }
}
/* Send _single_ skb sitting at the send head. This function requires
--
2.13.2
^ permalink raw reply related
* [RFC PATCH v1 1/5] tcp: Added callback for timed sender operations
From: Natale Patriciello @ 2017-07-28 19:59 UTC (permalink / raw)
To: David S . Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy
Cc: netdev, Ahmed Said, Natale Patriciello, Francesco Zampognaro,
Cesare Roseti
In-Reply-To: <20170728195919.10099-1-natale.patriciello@gmail.com>
Standard TCP is ACK-clocked, or in other words it must wait for ACKs after
sending a full window size bytes. However, in some particular cases, a
congestion control would like to be able to tell the TCP implementation when it
is possible to send segments through a timer. This patch adds the interface
(completely optional) between a congestion control and the TCP implementation.
Signed-off-by: Natale Patriciello <natale.patriciello@gmail.com>
Tested-by: Ahmed Said <ahmed.said@uniroma2.it>
---
include/net/tcp.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index be6223c586fa..bf661ccc53a2 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -939,6 +939,14 @@ struct tcp_congestion_ops {
/* get info for inet_diag (optional) */
size_t (*get_info)(struct sock *sk, u32 ext, int *attr,
union tcp_cc_info *info);
+ /* get the expiration time for the send timer (optional) */
+ unsigned long (*get_send_timer_exp_time)(struct sock *sk);
+ /* no data to transmit at the timer expiration (optional) */
+ void (*no_data_to_transmit)(struct sock *sk);
+ /* the send timer is expired (optional) */
+ void (*send_timer_expired)(struct sock *sk);
+ /* the TCP has sent some segments (optional) */
+ void (*segment_sent)(struct sock *sk, u32 sent);
char name[TCP_CA_NAME_MAX];
struct module *owner;
--
2.13.2
^ permalink raw reply related
* [RFC PATCH v1 0/5] TCP Wave
From: Natale Patriciello @ 2017-07-28 19:59 UTC (permalink / raw)
To: David S . Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy
Cc: netdev, Ahmed Said, Natale Patriciello, Francesco Zampognaro,
Cesare Roseti
Hi,
We are working on a new TCP congestion control algorithm, aiming at satisfying
new requirements coming from current networks. For instance, adaptation to
bandwidth/delay changes (due to mobility, dynamic switching, handover), and
optimal exploitation of very high link capacity and efficient transmission of
small objects, irrespective of the underlying link characteristics.
TCP Wave (TCPW) replaces the window-based transmission paradigm of the standard
TCP with a burst-based transmission, the ACK-clock scheduling with a
self-managed timer and the RTT-based congestion control loop with an Ack-based
Capacity and Congestion Estimation (ACCE) module. In non-technical words, it
sends data down the stack when its internal timer expires, and the timing of
the received ACKs contribute to updating this timer regularly.
We tried to add this new sender paradigm without deeply touching existing code.
In fact, we added four (optional) new congestion control functions:
+ /* get the expiration time for the send timer (optional) */
+ unsigned long (*get_send_timer_exp_time)(struct sock *sk);
+ /* no data to transmit at the timer expiration (optional) */
+ void (*no_data_to_transmit)(struct sock *sk);
+ /* the send timer is expired (optional) */
+ void (*send_timer_expired)(struct sock *sk);
+ /* the TCP has sent some segments (optional) */
+ void (*segment_sent)(struct sock *sk, u32 sent);
And a timer (tp->send_timer) which uses a send callback to push data down the
stack. If the first of these function, get_send_timer_exp_time, is not
implemented by the current congestion control, then the timer sending timer is
never set, therefore falling back to the old, ACK-clocked, behavior.
The TCPW module itself extensively make use of the existing infrastructure and
parameters to calculate its timer, plus some heuristics when it is not possible
to have trustworthy values from the network.
You can find more stuff related to TCPW (extended results, the test programs
used and the setup for the experiments, a document describing the algorithm in
detail and so on) at:
[1] http://tlcsat.uniroma2.it/tcpwave4linux/
We would greatly appreciate any feedback from you, comments, suggestions,
corrections and so on. Thank you for your attention.
Cesare, Francesco, Ahmed, Natale
Natale Patriciello (5):
tcp: Added callback for timed sender operations
tcp: Implemented the timing-based operations
tcp: PSH frames sent without timer involved
tcp: Add initial delay to allow data queueing
wave: Added basic version of TCP Wave
MAINTAINERS | 6 +
include/linux/tcp.h | 3 +
include/net/tcp.h | 8 +
net/ipv4/Kconfig | 16 +
net/ipv4/Makefile | 1 +
net/ipv4/tcp.c | 8 +-
net/ipv4/tcp_ipv4.c | 2 +
net/ipv4/tcp_output.c | 73 +++-
net/ipv4/tcp_wave.c | 914 ++++++++++++++++++++++++++++++++++++++++++++++++++
9 files changed, 1023 insertions(+), 8 deletions(-)
create mode 100644 net/ipv4/tcp_wave.c
--
2.13.2
^ permalink raw reply
* Re: refcount_t + (resend to wider audience)
From: Mark Salyzyn @ 2017-07-28 19:59 UTC (permalink / raw)
To: David Miller; +Cc: andrew, netdev, stable
In-Reply-To: <20170728.123139.1354715651678675641.davem@davemloft.net>
On 07/28/2017 12:31 PM, David Miller wrote:
> Sorry, even with this explanation this -stable require is completely
> and totally inappropriate.
Puts me between a rock and a hard place trying to address kernel
security issues. Should I instead file KASAN Use-After-Free reports on
stable kernels here for analysis by those with more wisdom to help
refine a more targeted fix?
For instance a dive on one of them did turn up
89e357d83c06b6fac581c3ca7f0ee3ae7e67109e which stopped an unbounded
refcounter by preventing multiple dump requests at the same time. But
the other 4 KASAN reports I focused on this week, we were not so lucky.
> You guys are really pushing things way too far with this refcount_t
> stuff, seriously.
First round ever on this, I guess I am missing some turmoil, history or
bad blood over refcount_t. Always fun to step on a landmine :-)
> NACK.
Please, guidance on where I can go from here.
Sincerely -- Mark Salyzyn
^ permalink raw reply
* Re: [PATCH net v2] net: phy: Correctly process PHY_HALTED in phy_stop_machine()
From: Marc Gonzalez @ 2017-07-28 19:53 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: David S. Miller, Andrew Lunn, Russell King, Mason
In-Reply-To: <20170728185836.28759-1-f.fainelli@gmail.com>
On 28/07/2017 20:58, Florian Fainelli wrote:
> Marc reported that he was not getting the PHY library adjust_link()
> callback function to run when calling phy_stop() + phy_disconnect()
> which does not indeed happen because we set the state machine to
> PHY_HALTED but we don't get to run it to process this state past that
> point.
>
> Fix this with a synchronous call to phy_state_machine() in order to have
> the state machine actually act on PHY_HALTED, set the PHY device's link
> down, turn the network device's carrier off and finally call the
> adjust_link() function.
>
> Reported-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v2:
>
> - reword subject and commit message based on changes
> - dropped flush_scheduled_work() since it is redundant
>
> drivers/net/phy/phy.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d0626bf5c540..5068c582d502 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -749,6 +749,9 @@ void phy_stop_machine(struct phy_device *phydev)
> if (phydev->state > PHY_UP && phydev->state != PHY_HALTED)
> phydev->state = PHY_UP;
> mutex_unlock(&phydev->lock);
> +
> + /* Now we can run the state machine synchronously */
> + phy_state_machine(&phydev->state_queue.work);
> }
>
> /**
Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Regards.
^ permalink raw reply
* Re: [PATCH net] ipv6: no need to return rt->dst.error if it is not null entry.
From: Roopa Prabhu @ 2017-07-28 19:52 UTC (permalink / raw)
To: David Ahern; +Cc: Cong Wang, Hangbin Liu, network dev
In-Reply-To: <ea0c0319-1b74-da8f-e307-2bf02e674119@gmail.com>
On Fri, Jul 28, 2017 at 10:39 AM, David Ahern <dsahern@gmail.com> wrote:
> On 7/28/17 11:13 AM, Roopa Prabhu wrote:
>> for fibmatch, my original intent was to return with an error code.
>> This is similar
>> to the ipv4 behavior. One option is to keep the check in there and put
>> the 'fibmatch'
>> condition around it. But, i do want to make sure that for the fibmatch case,
>> it does not return an error directly on an existing prohibit route
>> entry in the fib.
>> This is probably doable by checking for appropriate
>> net->ipv6.ip6_prohibit_entry entries.
>>
>
> IPv4 does not have the notion of null_entry or prohibit route entries
> which makes IPv4 and IPv6 inconsistent - something we really need to be
> avoiding from a user experience.
>
> We have the following cases:
>
> # ip -4 rule add to 172.16.60.0/24 prohibit
> # ip -4 route add prohibit 172.16.50.0/24
> # ip -6 rule add to 6000::/120 prohibit
> # ip -6 route add prohibit 5000::/120
>
>
> Behavior before Roopa's patch set:
> Rule match:
> # ip ro get 172.16.60.1
> RTNETLINK answers: Permission denied
>
> # ip -6 ro get 6000::1
> prohibit 6000::1 from :: dev lo proto kernel src 2001:db8::3 metric
> 4294967295 error -13 pref medium
>
> Route match:
> # ip ro get 172.16.50.1
> RTNETLINK answers: Permission denied
>
> # ip -6 ro get 5000::1
> prohibit 5000::1 from :: dev lo table red src 2001:db8::3 metric
> 1024 error -13 pref medium
>
>
> Behavior after Roopa's patch set:
> Rule match:
> # ip ro get 172.16.60.1
> RTNETLINK answers: Permission denied
>
> # ip -6 ro get 6000::1
> RTNETLINK answers: Permission denied
>
> Route match:
> # ip ro get 172.16.50.1
> RTNETLINK answers: Permission denied
>
> # ip -6 ro get 5000::1
> RTNETLINK answers: Permission denied
>
>
> So Roopa's fibmatch patches brings consistency between IPv4 and IPv6 at
> the cost of breaking backwards compatibility for IPv6 when the prohibit
> or blackhole routes are hit.
>
> If that is not acceptable, then let's wrap the change in 'if (fibmatch)'
> so that when fibmatch is requested we have consistency between IPv4 and
> IPv6 when it is set.
David, Thanks for listing all the cases and options.
for the route match fibmatch case, if a prohibit route entry exists
(added by user), I was hoping fibmatch can return that entry...
# ip -6 ro get fibmatch 5000::1
prohibit 5000::1 from :: dev lo
because the semantics of fibmatch is to return the matching route
entry if exists.
I am assuming that is possible with appropriate checks around the
dst.error check for fibmatch. what do you say ?
I need to verify if this can work for ipv4 the same way.
^ permalink raw reply
* Re: [PATCH net-next 4/4] net: dsa: lan9303: MDIO access phy registers directly
From: Egil Hjelmeland @ 2017-07-28 19:44 UTC (permalink / raw)
To: Florian Fainelli, Vivien Didelot, andrew, netdev, linux-kernel,
kernel
In-Reply-To: <1250382a-1c17-8e82-8c5c-fe14786c84db@gmail.com>
Den 28. juli 2017 19:05, skrev Florian Fainelli:
> On 07/28/2017 09:55 AM, Vivien Didelot wrote:
>> Hi Egil,
>>
>> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
>>
>>>>> +const struct lan9303_phy_ops lan9303_indirect_phy_ops = {
>>>>> + .phy_read = lan9303_indirect_phy_read,
>>>>> + .phy_write = lan9303_indirect_phy_write,
>>>>> +};
>>>>> +EXPORT_SYMBOL(lan9303_indirect_phy_ops);
>>>>
>>>> Isn't EXPORT_SYMBOL_GPL prefered over EXPORT_SYMBOL?
>>>
>>> I have no opinion. I just used the same variant as the other EXPORTS
>>> in the file.
>>
>> If there is no concern from others about this, LGTM too:
>
> Since the kernel module license is GPL, EXPORT_SYMBOL_GPL() would seem
> to be appropriate, which can be done as a subsequent patch.
>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>
I have no idea how these legalities work. But for the record,
I give consent to change to EXPORT_SYMBOL_GPL at any time.
Egil Hjelmeland
^ permalink raw reply
* Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings
From: Kurt Van Dijck @ 2017-07-28 19:41 UTC (permalink / raw)
To: Franklin S Cooper Jr
Cc: Oliver Hartkopp, Andrew Lunn, linux-kernel, devicetree, netdev,
linux-can, wg, mkl, robh+dt, quentin.schulz, sergei.shtylyov
In-Reply-To: <80c167a2-7eb5-8076-04fc-0989981a523d@ti.com>
>
> On 07/28/2017 01:33 PM, Oliver Hartkopp wrote:
> > Hi Kurt,
> >
> > On 07/28/2017 03:02 PM, Kurt Van Dijck wrote:
> >
> >>>> The word 'max-arbitration-bitrate' makes the difference very clear.
> >>>
> >>> I think you are mixing up ISO layer 1 and ISO layer 2.
> >>
> >> In order to provide higher data throughput without putting extra limits
> >> on transceiver & wire, the requirement for the round-trip delay to be
> >> within 1 bittime has been eliminated, but only for the data phase when
> >> arbitration is over.
> >> So layer 2 (CAN FD) has been adapted to circumvent the layer 1
> >> (transceiver + wire) limitations.
> >>
> >> In fact, the round-trip delay requirement never actually did matter for
> >> plain CAN during data bits either. CAN FD just makes use of that,
> >> but is therefore incompatible on the wire.
> >>
> >> I forgot the precise wording, but this is the principle that Bosch
> >> explained on the CAN conference in Nurnberg several years ago, or at
> >> least this is how I remembered it :-)
> >
> > I just checked an example for a CAN FD qualified transceiver
> >
> > http://www.nxp.com/products/automotive-products/energy-power-management/can-transceivers/high-speed-can-transceiver-with-standby-mode:TJA1044
> >
> >
> > where it states:
> >
> > The TJA1044T is specified for data rates up to 1 Mbit/s. Pending the
> > release of ISO11898-2:2016 including CAN FD and SAE-J2284-4/5,
> > additional timing parameters defining loop delay symmetry are specified
> > for the TJA1044GT and TJA1044GTK. This implementation enables reliable
> > communication in the CAN FD fast phase at data rates up to 5 Mbit/s.
> >
> > and
> >
> > TJA1044GT/TJA1044GTK
> >
> > - Timing guaranteed for data rates up to 5 Mbit/s
> > - Improved TXD to RXD propagation delay of 210 ns
Note the words "loop delay symmetry" and "CAN FD fast phase".
I didn't dig into the ISO's, but I read this as:
the TJA1044GT (not the TJA1044T) supports CAN FD data bitrate up to
5MBit/s. The overall (and thus arbitration) bitrate is max 1MBit/s.
What did I miss?
> >
> >> I haven't followed the developments of transceivers, but with the above
> >> principle in mind, it's obvious that any transceiver allows higher
> >> bitrates during the data segment because the TX-to-RX line delay must
> >> not scale with the bitrate.
> >> In reality, maybe not all transceivers will mention this in their
> >> datasheet.
> >>
> >> So whether you call it 'max-arbitration-bitrate' & 'max-data-bitrate'
> >> or 'max-bitrate' & 'max-data-bitrate' does not really matter (I prefer
> >> 1st) but you will one day need 2 bitrates.
> >
> > The question to me is whether it is right option to specify two bitrates
> > OR to specify one maximum bitrate and provide a property that a CAN FD
> > capable propagation delay is available.
> >
> > E.g.
> >
> > max-bitrate
> > max-data-bitrate
> >
> > or
> >
> > max-bitrate
> > canfd-capable // CAN FD capable propagation delay available
When you say: 'there is a CAN FD capable propagation delay available',
this actually means that you the transceiver specified a seperate
max-data-bitrate, but you don't mention what it is.
How can linux determine the max-data-bitrate to use?
> >
> >
> > I assume the optimized propagation delay is 'always on' as the
> > transceiver is not able to detect which kind of bits it is processing.
> > That's why I think providing two bitrates leads to a wrong view on the
> > transceiver.
>
> I agree with this.
I still disagree as I still think that CAN FD data phase (or fast phase)
has different round-trip delay (or "loop-delay symmetry") requirements
than the arbitration phase.
The CAN chip during data-phase does not need RX to follow TX as it does during
arbitration phase. The optimized propagation delay is 'always on' but
only applicable or sufficient during data phase. during arbitration
phase, you (the CAN FD chip) requires a better round-trip delay, and the
transceiver cannot deliver this.
>
> The transceiver is an analog device that needs to support faster
> switching frequency (FETs) including minimizing delay to support CAN-FD
> ie higher bitrate. From the transceiver perspective the bits for
> "arbitration" and "data" look exactly the same. Since it can't
> differentiate between the two (at the physical layer) then the actual
> limit isn't specific to which part/type of the CAN message is being
> sent. Rather its just a single overall max bitrate limit.
I must disagree here.
The transceiver is an analog device that performs 2 functions:
propagate tx bits to CAN wire, and propagate CAN wire state
(dominant/recesive) to rx bits.
I'll rephrase the above explanation to fit your argument:
During arbitration, both directions are required, and needs to propagate
within 1 bit time. The transceiver doesn't know, it just performs to
best effort.
During data, the round-trip timing requirement of layer2 is relaxed.
The transceiver still doesn't know, it still performs to best effort.
Due to the relaxed round-trip timing requirement, the same transceiver
can suddenly allow higher bitrates. The transceiver didn't change, the
requirement did change.
This is what I meant earlier with "layer2 has been adapted to circumvent
layer1 limitations"
Was I successfull in transcoding my thoughts onto email :-) ?
Kind regards,
Kurt
^ permalink raw reply
* Re: [PATCH V2 3/4] net-next: dsa: fix flow dissection
From: John Crispin @ 2017-07-28 19:40 UTC (permalink / raw)
To: Andrew Lunn
Cc: Vivien Didelot, Florian Fainelli, David S . Miller, netdev,
linux-kernel
In-Reply-To: <20170726151013.GD12049@lunn.ch>
On 26/07/17 17:10, Andrew Lunn wrote:
> On Fri, Jul 21, 2017 at 10:58:12AM +0200, John Crispin wrote:
>> RPS and probably other kernel features are currently broken on some if not
>> all DSA devices. The root cause of this is that skb_hash will call the
>> flow_dissector. At this point the skb still contains the magic switch header
>> and the skb->protocol field is not set up to the correct 802.3 value yet.
>> By the time the tag specific code is called, removing the header and
>> properly setting the protocol an invalid hash is already set. In the case
>> of the mt7530 this will result in all flows always having the same hash.
>>
>> This patch makes the flow dissector honour the nh and protocol offset
>> defined by the dsa tag driver thus fixing dissection, hashing and RPS.
>>
>> Signed-off-by: John Crispin <john@phrozen.org>
>> ---
>> net/core/flow_dissector.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index fc5fc4594c90..1268ae75c3b3 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -4,6 +4,7 @@
>> #include <linux/ip.h>
>> #include <linux/ipv6.h>
>> #include <linux/if_vlan.h>
>> +#include <net/dsa.h>
>> #include <net/ip.h>
>> #include <net/ipv6.h>
>> #include <net/gre.h>
>> @@ -440,6 +441,17 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>> skb->vlan_proto : skb->protocol;
>> nhoff = skb_network_offset(skb);
>> hlen = skb_headlen(skb);
>> +
>> + if (unlikely(netdev_uses_dsa(skb->dev))) {
>> + const struct dsa_device_ops *ops;
>> + u8 *p = (u8 *)data;
>> +
>> + ops = skb->dev->dsa_ptr->tag_ops;
>> + if (ops->hash_proto_off)
>> + proto = (u16)p[ops->hash_proto_off];
> Hi John
>
> Unfortunately, this is not generic enough to work for DSA and EDSA
> tagging. With these tagging protocols, the size of the tag depends on
> the presence or not of a VLAN header.
>
> To make this work for all tagging protocols, we are going to need to
> add an a new op to tag_ops.
>
> Andrew
Hi Andrew,
thanks for the feedback. should I add 2 callbacks for each of the 2
parameters ?
John
^ permalink raw reply
* Re: refcount_t + (resend to wider audience)
From: David Miller @ 2017-07-28 19:31 UTC (permalink / raw)
To: salyzyn; +Cc: andrew, netdev, stable
In-Reply-To: <48b0c119-5277-1eca-d5d6-f2539ad3a7e8@android.com>
From: Mark Salyzyn <salyzyn@android.com>
Date: Fri, 28 Jul 2017 11:07:53 -0700
> On 07/28/2017 10:41 AM, Andrew Lunn wrote:
>> On Fri, Jul 28, 2017 at 10:15:23AM -0700, Mark Salyzyn wrote:
>>> (Resend to wider audience to comply with
>>> Documentation/networking/netdev-FAQ.txt)
>>>
>>> Please apply/backport the following upstream feature and followup
>>> grouped fixes patches to the stable trees (expect included in at least
>>> 3.10.y, 3.18.y, 4.4.y and 4.9.y):
>> .. _stable_kernel_rules:
>>
>> Hi Mark
>>
>> Everything you ever wanted to know about Linux -stable releases
>> ===============================================================
>>
>> Rules on what kind of patches are accepted, and which ones are not,
>> into the
>> "-stable" tree:
>>
>> - It must be obviously correct and tested.
>> - It cannot be bigger than 100 lines, with context.
>>
>> The first patch you list is 342 lines. The second one is 634.
>>
>> Please could you read the rules and then provide some justification
>> for ignoring many of the rules.
>>
>> Andrew
>
> The first four patches add a new dependent upstream API and type,
> refcount_t. New APIs will notoriously cause a large number of lines to
> be adjusted. They are complete (ToT will/should match stable),
> orthogonal, and without CONFIG_REFCOUNT_FULL completely inert in all
> places where atomic_t reference counters used, and are replaced with
> refcount_t in the followup patches that take advantage of this new
> type.
>
> The first four do _nothing_ at all to the kernel as-is, but represent
> a dependency for the following changes.
>
> The remaining patches (for the most part) take advantage of this new
> API to mostly fix, or report/warn when they can not, Use-After-Free
> (KASAN) bugs which can lead to root attack exploits. atomic_t are
> subject to unbounded attacks, refcount_t are relatively immune to
> unbounded attacks. It is admittedly not a complete fix, but greatly
> reduce the chances of the security issues. The recommendation is to
> turn on CONFIG_REFCOUNT_FULL, but that is a decision to balance
> between security and performance.
>
> For any platform that requires the latest security updates, refcount_t
> is going to be a requirement. I urge you to overlook the first four
> patch sizes because of their status as an orthogonal type and API,
> necessary dependency for security improvements.
Sorry, even with this explanation this -stable require is completely
and totally inappropriate.
You guys are really pushing things way too far with this refcount_t
stuff, seriously.
NACK.
^ permalink raw reply
* Re: STABLE: net: reduce skb_warn_bad_offload() noise
From: David Miller @ 2017-07-28 19:30 UTC (permalink / raw)
To: salyzyn; +Cc: stable, netdev
In-Reply-To: <4b5d320a-a12e-b27b-cc58-332614684bf2@android.com>
From: Mark Salyzyn <salyzyn@android.com>
Date: Fri, 28 Jul 2017 10:29:57 -0700
> Please backport the upstream patch to the stable trees (including
> 3.10.y, 3.18.y, 4.4.y and 4.9.y):
>
> b2504a5dbef3305ef41988ad270b0e8ec289331c net: reduce
> skb_warn_bad_offload() noise
>
> Impacting performance or creating unnecessary alarm, and will result
> in kernel panic for panic_on_warn configuration.
Yeah this is fine.
^ permalink raw reply
* Re: refcount_t + (resend to wider audience)
From: David Miller @ 2017-07-28 19:29 UTC (permalink / raw)
To: salyzyn; +Cc: netdev, stable
In-Reply-To: <adaee9d9-1d35-921c-36b3-e32e6c8ce9e3@android.com>
From: Mark Salyzyn <salyzyn@android.com>
Date: Fri, 28 Jul 2017 10:15:23 -0700
> (Resend to wider audience to comply with
> Documentation/networking/netdev-FAQ.txt)
>
> Please apply/backport the following upstream feature and followup
> grouped fixes patches to the stable trees (expect included in at least
> 3.10.y, 3.18.y, 4.4.y and 4.9.y):
I totally disagree, this is too invasive for -stable.
No way.
^ permalink raw reply
* Re: [RFC net-next 0/6] tcp: remove prequeue and header prediction
From: Yuchung Cheng @ 2017-07-28 19:19 UTC (permalink / raw)
To: Florian Westphal
Cc: netdev, Neal Cardwell, Eric Dumazet, Soheil Hassas Yeganeh,
Wei Wang, Lawrence Brakmo
In-Reply-To: <20170727233117.29695-1-fw@strlen.de>
On Thu, Jul 27, 2017 at 4:31 PM, Florian Westphal <fw@strlen.de> wrote:
>
> This RFC removes tcp prequeueing and header prediction support.
>
> After a hallway discussion with Eric Dumazet some
> maybe-not-so-useful-anymore TCP stack features came up, HP and
> Prequeue among these.
>
> So this RFC proposes to axe both.
>
> In brief, TCP prequeue assumes a single-process-blocking-read
> design, which is not that common anymore, and the most frequently
> used high-performance networking program that does this is netperf :)
>
> With more commong (e)poll designs, prequeue doesn't work.
>
> The idea behind prequeueing isn't so bad in itself; it moves
> part of tcp processing -- including ack processing (including
> retransmit queue processing) into process context.
> However, removing it would not just avoid some code, for most
> programs it elimiates dead code.
>
> As processing then always occurs in BH context, it would allow us
> to experiment e.g. with bulk-freeing of skb heads when a packet acks
> data on the retransmit queue.
>
> Header prediction is also less useful nowadays.
> For packet trains, GRO will aggregate packets so we do not get
> a per-packet benefit.
> Header prediction will also break down with light packet loss due to SACK.
>
> So, In short: What do others think?
+2 for this move. Will review the patches soon.
>
> Florian Westphal (6):
> tcp: remove prequeue support
> tcp: reindent two spots after prequeue removal
> tcp: remove low_latency sysctl
> tcp: remove header prediction
> tcp: remove CA_ACK_SLOWPATH
> tcp: remove unused mib counters
>
> Documentation/networking/ip-sysctl.txt | 7
> include/linux/tcp.h | 15 -
> include/net/tcp.h | 40 ----
> include/uapi/linux/snmp.h | 8
> net/ipv4/proc.c | 8
> net/ipv4/sysctl_net_ipv4.c | 3
> net/ipv4/tcp.c | 109 -----------
> net/ipv4/tcp_input.c | 303 +++------------------------------
> net/ipv4/tcp_ipv4.c | 63 ------
> net/ipv4/tcp_minisocks.c | 3
> net/ipv4/tcp_output.c | 2
> net/ipv4/tcp_timer.c | 12 -
> net/ipv4/tcp_westwood.c | 31 ---
> net/ipv6/tcp_ipv6.c | 3
> 14 files changed, 43 insertions(+), 564 deletions(-)
>
^ permalink raw reply
* Re: Long stalls creating a new netns after a netns with a SMB client exits
From: David Ahern @ 2017-07-28 19:16 UTC (permalink / raw)
To: Rolf Neugebauer, Cong Wang; +Cc: Linux Kernel Network Developers
In-Reply-To: <CA+pO-2cUUy=_Krwnvt5GL-DUsu+oBOHwhF-TE8sqW=7PFMBrbA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]
On 7/28/17 12:58 PM, Rolf Neugebauer wrote:
>>> I can readily reproduce this on 4.9.39, 4.11.12 and another user
>>> repro-ed it on 4.12.3. It seems to happen every time. At least one
>>> user reported issues with NFS mounts as well, but we were not able to
>>> reproduce it. It's not clear to me if this is directly related to
>>> 'mount.cifs' or if that just happens to reliably repro it.
>>
>> OK, so commit d747a7a51b00984127a88113c does not help this case
>> either.
>
> d747a7a51b009("tcp: reset sk_rx_dst in tcp_disconnect()") indeed seems
> a different issue. As I understand that actually caused the ref count
> never to get decremented, while here eventually some cleanup kicks in
> after a long timeout.
It could be a dst is cached on a socket and does not get cleared until
the socket time outs are done.
Test that theory by something like this for IPv4 TCP (similar change for
UDP if the client is UDP based):
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 3a19ea28339f..37db087b6c97 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1855,7 +1855,7 @@ void inet_sk_rx_dst_set(struct sock *sk, const
struct sk_buff *skb)
{
struct dst_entry *dst = skb_dst(skb);
- if (dst && dst_hold_safe(dst)) {
+ if (0 && dst && dst_hold_safe(dst)) {
sk->sk_rx_dst = dst;
inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
}
>
> I'll also try if I can get some traces out of dev_hold()/dev_put().
Attached patch puts tracepoints in dev_hold / dev_put; very useful for
debugging cases like this. Use perf record and perf script.
[-- Attachment #2: 0001-Add-tracepoints-to-dev_hold-and-dev_put.patch --]
[-- Type: text/plain, Size: 2875 bytes --]
From 068b1b8362ec5fd1b9dffdbd6e84474ada2eb829 Mon Sep 17 00:00:00 2001
From: David Ahern <dsa@cumulusnetworks.com>
Date: Thu, 11 Feb 2016 02:40:12 -0800
Subject: [PATCH] Add tracepoints to dev_hold and dev_put
Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
include/linux/netdevice.h | 6 ++++++
include/trace/events/net.h | 38 ++++++++++++++++++++++++++++++++++++++
net/core/dev.c | 21 +++++++++++++++++++++
3 files changed, 65 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 219f53c30cb3..7ef6fc672dfb 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3193,6 +3193,7 @@ extern int netdev_budget;
/* Called by rtnetlink.c:rtnl_unlock() */
void netdev_run_todo(void);
+#if 0
/**
* dev_put - release reference to device
* @dev: network device
@@ -3214,6 +3215,11 @@ static inline void dev_hold(struct net_device *dev)
{
this_cpu_inc(*dev->pcpu_refcnt);
}
+#else
+void dev_put(struct net_device *dev);
+void dev_hold(struct net_device *dev);
+
+#endif
/* Carrier loss detection, dial on demand. The functions netif_carrier_on
* and _off may be called from IRQ context, but it is caller
diff --git a/include/trace/events/net.h b/include/trace/events/net.h
index 49cc7c3de252..9ed73dfe9d09 100644
--- a/include/trace/events/net.h
+++ b/include/trace/events/net.h
@@ -236,6 +236,44 @@ DEFINE_EVENT(net_dev_rx_verbose_template, netif_rx_ni_entry,
TP_ARGS(skb)
);
+TRACE_EVENT(dev_put,
+
+ TP_PROTO(struct net_device *dev),
+
+ TP_ARGS(dev),
+
+ TP_STRUCT__entry(
+ __string( name, dev->name )
+ __field( int, refcnt )
+ ),
+
+ TP_fast_assign(
+ __assign_str(name, dev->name);
+ __entry->refcnt = netdev_refcnt_read(dev);
+ ),
+
+ TP_printk("dev=%s refcnt %d", __get_str(name), __entry->refcnt)
+);
+
+TRACE_EVENT(dev_hold,
+
+ TP_PROTO(struct net_device *dev),
+
+ TP_ARGS(dev),
+
+ TP_STRUCT__entry(
+ __string( name, dev->name )
+ __field( int, refcnt )
+ ),
+
+ TP_fast_assign(
+ __assign_str(name, dev->name);
+ __entry->refcnt = netdev_refcnt_read(dev);
+ ),
+
+ TP_printk("dev=%s refcnt %d", __get_str(name), __entry->refcnt)
+);
+
#endif /* _TRACE_NET_H */
/* This part must be outside protection */
diff --git a/net/core/dev.c b/net/core/dev.c
index f1284835b8c9..99ac067afd18 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8117,3 +8117,24 @@ static int __init net_dev_init(void)
}
subsys_initcall(net_dev_init);
+
+
+void dev_put(struct net_device *dev)
+{
+ trace_dev_put(dev);
+ this_cpu_dec(*dev->pcpu_refcnt);
+}
+EXPORT_SYMBOL(dev_put);
+
+/**
+ * dev_hold - get reference to device
+ * @dev: network device
+ *
+ * Hold reference to device to keep it from being freed.
+ */
+void dev_hold(struct net_device *dev)
+{
+ trace_dev_hold(dev);
+ this_cpu_inc(*dev->pcpu_refcnt);
+}
+EXPORT_SYMBOL(dev_hold);
--
2.1.4
^ permalink raw reply related
* Re: [v4.12 regression] netns: NULL deref in fib_sync_down_dev()
From: Ido Schimmel @ 2017-07-28 19:08 UTC (permalink / raw)
To: Michał Mirosław; +Cc: Cong Wang, Linux Kernel Network Developers
In-Reply-To: <20170728180437.s4pwdyguihmgjrjp@qmqm.qmqm.pl>
On Fri, Jul 28, 2017 at 08:04:37PM +0200, Michał Mirosław wrote:
> On Fri, Jul 28, 2017 at 08:36:02PM +0300, Ido Schimmel wrote:
> > On Fri, Jul 28, 2017 at 10:28:16AM -0700, Cong Wang wrote:
> > > On Fri, Jul 28, 2017 at 9:43 AM, Ido Schimmel <idosch@idosch.org> wrote:
> > > > On Fri, Jul 28, 2017 at 06:00:47PM +0200, Michał Mirosław wrote:
> > > >> Dear NetDevs,
> > > >>
> > > >> Before I go to bisecting, have you seen a following NULL dereference,
> > > >> yet? Where should I start looking? It is triggered by deleting netns
> > > >> (cut-down script attached - triggers every time). This was working
> > > >> correctly under v4.11.x.
> > > > Thanks for the report. I just reproduced this on my system. I believe
> > > > the problem is a missing NULL check for 'in_dev' in
> > > > call_fib_nh_notifiers(). I'll test a fix.
> > > But your commit 982acb97560c8118c2109504a22b0d78a580547d
> > > is merged in v4.11-rc1. How could 4.11.x work correctly?
> > It doesn't. I just reproduced this on v4.11.
>
> Thanks for looking into this. I was sure that I ran v4.11.7 last time,
> but it turns out I worked on this earlier than that. I'll be glad to
> test patches for this issue when you have it.
I've a working patch, but I tried to understand why we didn't see it
until now. I believe the problem is the fact that you have an interface
with no IP address and a route pointing to it.
When it goes down, inetdev_destroy() is called, which sets dev->ip_ptr to
NULL. Then the netdev notification block in the FIB is called and the
NULL dereference occurs.
If an IP address was assigned, then before NULLing dev->ip_ptr, all the
IP addresses would be flushed and the inetaddr notification block in the
FIB would be called, which in turn would flush all the routes. Since all
the routes were already flushed, no NULL dereference would occur when
the FIB's netdev notification block is called.
I'll post the patch shortly.
Thanks again.
^ permalink raw reply
* [PATCH net v2] net: phy: Correctly process PHY_HALTED in phy_stop_machine()
From: Florian Fainelli @ 2017-07-28 18:58 UTC (permalink / raw)
To: netdev
Cc: davem, andrew, slash.tmp, marc_gonzalez, rmk+kernel,
Florian Fainelli
Marc reported that he was not getting the PHY library adjust_link()
callback function to run when calling phy_stop() + phy_disconnect()
which does not indeed happen because we set the state machine to
PHY_HALTED but we don't get to run it to process this state past that
point.
Fix this with a synchronous call to phy_state_machine() in order to have
the state machine actually act on PHY_HALTED, set the PHY device's link
down, turn the network device's carrier off and finally call the
adjust_link() function.
Reported-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:
- reword subject and commit message based on changes
- dropped flush_scheduled_work() since it is redundant
drivers/net/phy/phy.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index d0626bf5c540..5068c582d502 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -749,6 +749,9 @@ void phy_stop_machine(struct phy_device *phydev)
if (phydev->state > PHY_UP && phydev->state != PHY_HALTED)
phydev->state = PHY_UP;
mutex_unlock(&phydev->lock);
+
+ /* Now we can run the state machine synchronously */
+ phy_state_machine(&phydev->state_queue.work);
}
/**
--
2.9.3
^ permalink raw reply related
* Re: Long stalls creating a new netns after a netns with a SMB client exits
From: Rolf Neugebauer @ 2017-07-28 18:58 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers, Rolf Neugebauer
In-Reply-To: <CAM_iQpW+gSgCDWdGoHvN0wObda_g40FcyCBem5VVJ4XLHNMRaQ@mail.gmail.com>
On Fri, Jul 28, 2017 at 6:49 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> Hello,
>
> On Fri, Jul 28, 2017 at 9:47 AM, Rolf Neugebauer
> <rolf.neugebauer@docker.com> wrote:
>> Creating the new namespace is stalling for around 200 seconds and
>> there 20 odd messages on the console, like:
>>
>> [ 67.372603] unregister_netdevice: waiting for lo to become free.
>> Usage count = 1
>>
>
> Sounds like another netdev refcnt leak.
I don't think it's a leak as such because the system eventually
recovers after around 200 seconds.
>
>> Adding a 'sleep 1' before deleting the original network namespace
>> "solves" the issue, but that doesn't sound like a good fix. Not using
>> unmount also does not help (understandable).
>
>
> Interesting, if sleeping for 1sec help, why did you see the stall for
> 200sec? The "leak" should go away eventually without 'sleep 1',
> right?
Yes. I suspect, that with a sleep some cleanup code (maybe umount)
gets run and the ref count gets decremented within the second. Without
the sleep, something gets yanked, and whatever operation needs to be
done can't get performed, times out after 200s and then the ref count
gets decremented.
>
>>
>> While the creation of the new namespace is stalled, I used 'sysrq' a
>> few times to dump the work queues. There is an example below. Also,
>> the hung task detection kicks in after 120 seconds (also below)
>
> Yeah, the net_mutex is held by cleanup_net().
>
>>
>> I can readily reproduce this on 4.9.39, 4.11.12 and another user
>> repro-ed it on 4.12.3. It seems to happen every time. At least one
>> user reported issues with NFS mounts as well, but we were not able to
>> reproduce it. It's not clear to me if this is directly related to
>> 'mount.cifs' or if that just happens to reliably repro it.
>
> OK, so commit d747a7a51b00984127a88113c does not help this case
> either.
d747a7a51b009("tcp: reset sk_rx_dst in tcp_disconnect()") indeed seems
a different issue. As I understand that actually caused the ref count
never to get decremented, while here eventually some cleanup kicks in
after a long timeout.
>
>>
>> It would be great if someone more familiar with the code could take a
>> look. I'm happy to provide additional info (perf traces etc) or test
>> patches if needed.
>>
>
> The last time I debugged this kind of netdev refcnt leak problem,
> I added a few trace_printk() to dev_hold() and dev_put(),
> so you can try it too. I will see if I can use your reproducer
> here.
The last time I encountered the same symptoms were here
http://www.spinics.net/lists/netdev/msg403433.html but this had an
entirely different cause.
I'll also try if I can get some traces out of dev_hold()/dev_put().
Rolf
>
> Thanks.
^ permalink raw reply
* Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
From: Måns Rullgård @ 2017-07-28 18:56 UTC (permalink / raw)
To: Marc Gonzalez; +Cc: Florian Fainelli, David S. Miller, netdev, Linux ARM, Mason
In-Reply-To: <c1bf54ee-8634-23a1-abda-55bc3fafe2a8@sigmadesigns.com>
Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
> On 28/07/2017 18:17, Måns Rullgård wrote:
>
>> Marc Gonzalez wrote:
>>
>>> ndo_stop breaks RX in a way that ndo_open is unable to undo.
>>
>> Please elaborate. Why can't it be fixed in a less heavy-handed way?
>
> I'm not sure what "elaborate" means. After we've been through
> ndo_stop once, the board can send packets, but it doesn't see
> any replies from remote systems. RX is wedged.
So you say, but you have not explained why this happens. Until we know
why, we can't decide on the proper fix.
> I think ndo_stop is rare enough an event that doing a full
> reset is not an issue, in terms of performance.
Performance isn't the issue. Doing the right thing is.
> Also I will need this infrastructure anyway for suspend/resume
> support.
> It might also make sense to put the HW in reset at close
> (to save power). I will try measuring the power savings,
> if any.
>
>>> Work around the issue by resetting the HW in ndo_open.
>>> This will provide the basis for suspend/resume support.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>> drivers/net/ethernet/aurora/nb8800.c | 40 +++++++++++++++++-------------------
>>> drivers/net/ethernet/aurora/nb8800.h | 1 +
>>> 2 files changed, 20 insertions(+), 21 deletions(-)
>>
>> I'm pretty sure this doesn't preserve everything it should.
>
> Hmmm, we're supposed to start fresh ("full reset").
> What could there be to preserve?
> You mentioned flow control and multicast elsewhere.
> I will take a closer look. Thanks for the heads up.
Yes, those settings are definitely lost with your patch. Now I'm not
sure whether the networking core expects these to survive a stop/start
cycle, so please check that. There might also be other less obvious
things that need to be preserved.
--
Måns Rullgård
^ permalink raw reply
* Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings
From: Franklin S Cooper Jr @ 2017-07-28 18:53 UTC (permalink / raw)
To: Oliver Hartkopp, Andrew Lunn, linux-kernel, devicetree, netdev,
linux-can, wg, mkl, robh+dt, quentin.schulz, sergei.shtylyov,
dev.kurt
In-Reply-To: <c7d4c364-1ed9-ca73-2571-d04297ac3788@hartkopp.net>
On 07/28/2017 01:33 PM, Oliver Hartkopp wrote:
> Hi Kurt,
>
> On 07/28/2017 03:02 PM, Kurt Van Dijck wrote:
>
>>>> The word 'max-arbitration-bitrate' makes the difference very clear.
>>>
>>> I think you are mixing up ISO layer 1 and ISO layer 2.
>>
>> In order to provide higher data throughput without putting extra limits
>> on transceiver & wire, the requirement for the round-trip delay to be
>> within 1 bittime has been eliminated, but only for the data phase when
>> arbitration is over.
>> So layer 2 (CAN FD) has been adapted to circumvent the layer 1
>> (transceiver + wire) limitations.
>>
>> In fact, the round-trip delay requirement never actually did matter for
>> plain CAN during data bits either. CAN FD just makes use of that,
>> but is therefore incompatible on the wire.
>>
>> I forgot the precise wording, but this is the principle that Bosch
>> explained on the CAN conference in Nurnberg several years ago, or at
>> least this is how I remembered it :-)
>
> I just checked an example for a CAN FD qualified transceiver
>
> http://www.nxp.com/products/automotive-products/energy-power-management/can-transceivers/high-speed-can-transceiver-with-standby-mode:TJA1044
>
>
> where it states:
>
> The TJA1044T is specified for data rates up to 1 Mbit/s. Pending the
> release of ISO11898-2:2016 including CAN FD and SAE-J2284-4/5,
> additional timing parameters defining loop delay symmetry are specified
> for the TJA1044GT and TJA1044GTK. This implementation enables reliable
> communication in the CAN FD fast phase at data rates up to 5 Mbit/s.
>
> and
>
> TJA1044GT/TJA1044GTK
>
> - Timing guaranteed for data rates up to 5 Mbit/s
> - Improved TXD to RXD propagation delay of 210 ns
>
>> I haven't followed the developments of transceivers, but with the above
>> principle in mind, it's obvious that any transceiver allows higher
>> bitrates during the data segment because the TX-to-RX line delay must
>> not scale with the bitrate.
>> In reality, maybe not all transceivers will mention this in their
>> datasheet.
>>
>> So whether you call it 'max-arbitration-bitrate' & 'max-data-bitrate'
>> or 'max-bitrate' & 'max-data-bitrate' does not really matter (I prefer
>> 1st) but you will one day need 2 bitrates.
>
> The question to me is whether it is right option to specify two bitrates
> OR to specify one maximum bitrate and provide a property that a CAN FD
> capable propagation delay is available.
>
> E.g.
>
> max-bitrate
> max-data-bitrate
>
> or
>
> max-bitrate
> canfd-capable // CAN FD capable propagation delay available
>
>
> I assume the optimized propagation delay is 'always on' as the
> transceiver is not able to detect which kind of bits it is processing.
> That's why I think providing two bitrates leads to a wrong view on the
> transceiver.
I agree with this.
The transceiver is an analog device that needs to support faster
switching frequency (FETs) including minimizing delay to support CAN-FD
ie higher bitrate. From the transceiver perspective the bits for
"arbitration" and "data" look exactly the same. Since it can't
differentiate between the two (at the physical layer) then the actual
limit isn't specific to which part/type of the CAN message is being
sent. Rather its just a single overall max bitrate limit.
>
> Regards,
> Oliver
>
^ permalink raw reply
* Re: [PATCH net] net: phy: Run state machine to completion
From: Florian Fainelli @ 2017-07-28 18:50 UTC (permalink / raw)
To: Marc Gonzalez, David S. Miller, netdev
Cc: Russell King, Zach Brown, Nathan Sullivan, Andrew Lunn, LKML,
Mason, Mans Rullgard
In-Reply-To: <69ecff09-3822-31b3-99d2-ebd5ca888e7f@sigmadesigns.com>
On 07/28/2017 03:57 AM, Marc Gonzalez wrote:
> On 26/07/2017 21:24, Florian Fainelli wrote:
>
>> Marc reported that he was not getting the PHY library adjust_link()
>> callback function to run when calling phy_stop() + phy_disconnect()
>> which does not indeed happen because we don't make sure we flush the
>> PHYLIB delayed work and let it run to completion. We also need to have
>> a synchronous call to phy_state_machine() in order to have the state
>> machine actually act on PHY_HALTED, set the PHY device's link down, turn
>> the network device's carrier off and finally call the adjust_link()
>> function.
>>
>> Reported-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> Fixes: a390d1f379cf ("phylib: convert state_queue work to delayed_work")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> David, this dates back from before the commit mentioned in Fixes but it would
>> be hard to backport to earlier kernels if we flagged the original design flaw
>> that used timers. Also, I am not clear on the timer API whether there was a way
>> to ensure timers would run to completion before they would be cancelled.
>>
>> Marc, please add your Signed-off-by tag since you contributed the second line.
>>
>> Thanks!
>>
>> drivers/net/phy/phy.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index d0626bf5c540..30e7c43e0d87 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -743,12 +743,17 @@ void phy_trigger_machine(struct phy_device *phydev, bool sync)
>> */
>> void phy_stop_machine(struct phy_device *phydev)
>> {
>> + /* Run the state machine to completion */
>> + flush_delayed_work(&phydev->state_queue);
>> cancel_delayed_work_sync(&phydev->state_queue);
>>
>> mutex_lock(&phydev->lock);
>> if (phydev->state > PHY_UP && phydev->state != PHY_HALTED)
>> phydev->state = PHY_UP;
>> mutex_unlock(&phydev->lock);
>> +
>> + /* Now we can run the state machine synchronously */
>> + phy_state_machine(&phydev->state_queue.work);
>
> Hello Florian,
>
> Sorry, I was AFK for a couple of days.
>
> Thanks for putting the patch together!
>
> I added dump_stack() at the top of phy_state_machine() to better
> understand what's going on.
>
> Action: set link down on the command-line
>
> Results:
>
> 1) PHY in interrupt mode
>
> [ 136.238889] [<c0378518>] (phy_state_machine) from [<c0378a58>] (phy_stop_machine+0x4c/0x50)
> [ 136.247285] [<c0378a58>] (phy_stop_machine) from [<c0379c30>] (phy_disconnect+0x20/0x34)
> [ 136.255420] [<c0379c30>] (phy_disconnect) from [<c037d984>] (nb8800_stop+0x84/0xa0)
> [ 136.263118] [<c037d984>] (nb8800_stop) from [<c03f52a4>] (__dev_close_many+0x88/0xd0)
> [ 136.270989] [<c03f52a4>] (__dev_close_many) from [<c03f540c>] (__dev_close+0x24/0x38)
> [ 136.278862] [<c03f540c>] (__dev_close) from [<c03fdd98>] (__dev_change_flags+0x94/0x144)
> [ 136.286997] [<c03fdd98>] (__dev_change_flags) from [<c03fde60>] (dev_change_flags+0x18/0x48)
> [ 136.295481] [<c03fde60>] (dev_change_flags) from [<c0463350>] (devinet_ioctl+0x6e0/0x7a0)
> [ 136.303702] [<c0463350>] (devinet_ioctl) from [<c04657e0>] (inet_ioctl+0x194/0x1c0)
> [ 136.311400] [<c04657e0>] (inet_ioctl) from [<c03de2f8>] (sock_ioctl+0x148/0x2fc)
> [ 136.318837] [<c03de2f8>] (sock_ioctl) from [<c01dbe80>] (do_vfs_ioctl+0x9c/0x8e8)
> [ 136.326361] [<c01dbe80>] (do_vfs_ioctl) from [<c01dc700>] (SyS_ioctl+0x34/0x5c)
> [ 136.333710] [<c01dc700>] (SyS_ioctl) from [<c01076c0>] (ret_fast_syscall+0x0/0x3c)
> [ 136.341357] nb8800 26000.ethernet eth0: Link is Down
>
> Above, phy_state_machine() is called synchronously from phy_stop_machine().
>
>
> 2) PHY in polling mode
>
> [ 54.045752] CPU: 3 PID: 892 Comm: kworker/3:1 Not tainted 4.13.0-rc1 #7
> [ 54.056435] Workqueue: events_power_efficient phy_state_machine
> [ 54.085399] [<c0378518>] (phy_state_machine) from [<c012e404>] (process_one_work+0x1d4/0x3ec)
> [ 54.093969] [<c012e404>] (process_one_work) from [<c012f250>] (worker_thread+0x268/0x554)
> [ 54.102189] [<c012f250>] (worker_thread) from [<c0133fd4>] (kthread+0x108/0x138)
> [ 54.109623] [<c0133fd4>] (kthread) from [<c0107778>] (ret_from_fork+0x14/0x3c)
> [ 54.116918] nb8800 26000.ethernet eth0: Link is Down
>
> [ 54.122100] CPU: 2 PID: 973 Comm: ip Not tainted 4.13.0-rc1 #7
> [ 54.155001] [<c0378518>] (phy_state_machine) from [<c0378a58>] (phy_stop_machine+0x4c/0x50)
> [ 54.163397] [<c0378a58>] (phy_stop_machine) from [<c0379c30>] (phy_disconnect+0x20/0x34)
> [ 54.171532] [<c0379c30>] (phy_disconnect) from [<c037d984>] (nb8800_stop+0x84/0xa0)
> [ 54.179231] [<c037d984>] (nb8800_stop) from [<c03f52a4>] (__dev_close_many+0x88/0xd0)
> [ 54.187103] [<c03f52a4>] (__dev_close_many) from [<c03f540c>] (__dev_close+0x24/0x38)
> [ 54.194975] [<c03f540c>] (__dev_close) from [<c03fdd98>] (__dev_change_flags+0x94/0x144)
> [ 54.203109] [<c03fdd98>] (__dev_change_flags) from [<c03fde60>] (dev_change_flags+0x18/0x48)
> [ 54.211594] [<c03fde60>] (dev_change_flags) from [<c0463350>] (devinet_ioctl+0x6e0/0x7a0)
> [ 54.219816] [<c0463350>] (devinet_ioctl) from [<c04657e0>] (inet_ioctl+0x194/0x1c0)
> [ 54.227513] [<c04657e0>] (inet_ioctl) from [<c03de2f8>] (sock_ioctl+0x148/0x2fc)
> [ 54.234950] [<c03de2f8>] (sock_ioctl) from [<c01dbe80>] (do_vfs_ioctl+0x9c/0x8e8)
> [ 54.242473] [<c01dbe80>] (do_vfs_ioctl) from [<c01dc700>] (SyS_ioctl+0x34/0x5c)
> [ 54.249821] [<c01dc700>] (SyS_ioctl) from [<c01076c0>] (ret_fast_syscall+0x0/0x3c)
>
> Above() phy_state_machine() is called twice
> - first supposedly after flush_delayed_work() => calls adjust_link()
> - a second time by the explicit call => probably no-op
>
> Therefore, my reasoning is that flush_delayed_work() is
> unnecessary. The explicit call to phy_state_machine() at
> the end of phy_stop_machine() will handle all cases.
>
> What do you think?
You are right I also just tested PHY_POLL and PHY_IGNORE_INTERRUPT and
only the synchronous call is necessary. Will re-post a v2 shortly.
>
> Testing without flush_delayed_work() in polling mode:
>
> [ 58.819100] CPU: 0 PID: 968 Comm: ip Not tainted 4.13.0-rc1 #10
> [ 58.852098] [<c0378518>] (phy_state_machine) from [<c0378a50>] (phy_stop_machine+0x44/0x48)
> [ 58.860494] [<c0378a50>] (phy_stop_machine) from [<c0379c28>] (phy_disconnect+0x20/0x34)
> [ 58.868629] [<c0379c28>] (phy_disconnect) from [<c037d97c>] (nb8800_stop+0x84/0xa0)
> [ 58.876328] [<c037d97c>] (nb8800_stop) from [<c03f529c>] (__dev_close_many+0x88/0xd0)
> [ 58.884198] [<c03f529c>] (__dev_close_many) from [<c03f5404>] (__dev_close+0x24/0x38)
> [ 58.892071] [<c03f5404>] (__dev_close) from [<c03fdd90>] (__dev_change_flags+0x94/0x144)
> [ 58.900206] [<c03fdd90>] (__dev_change_flags) from [<c03fde58>] (dev_change_flags+0x18/0x48)
> [ 58.908690] [<c03fde58>] (dev_change_flags) from [<c0463348>] (devinet_ioctl+0x6e0/0x7a0)
> [ 58.916911] [<c0463348>] (devinet_ioctl) from [<c04657d8>] (inet_ioctl+0x194/0x1c0)
> [ 58.924608] [<c04657d8>] (inet_ioctl) from [<c03de2f0>] (sock_ioctl+0x148/0x2fc)
> [ 58.932046] [<c03de2f0>] (sock_ioctl) from [<c01dbe80>] (do_vfs_ioctl+0x9c/0x8e8)
> [ 58.939569] [<c01dbe80>] (do_vfs_ioctl) from [<c01dc700>] (SyS_ioctl+0x34/0x5c)
> [ 58.946917] [<c01dc700>] (SyS_ioctl) from [<c01076c0>] (ret_fast_syscall+0x0/0x3c)
> [ 58.954609] nb8800 26000.ethernet eth0: Link is Down
>
> Above, the call to adjust_link() occurs from the explicit call
> to phy_state_machine()
>
> Regards.
>
--
Florian
^ permalink raw reply
* Re: [PATCH net] xgene: Don't fail probe, if there is no clk resource for SGMII interfaces
From: Iyappan Subramanian @ 2017-07-28 18:40 UTC (permalink / raw)
To: Tom Bogendoerfer
Cc: Laura Abbott, Keyur Chudgar, Quan Nguyen, netdev,
linux-kernel@vger.kernel.org
In-Reply-To: <20170728142318.GA20368@alpha.franken.de>
On Fri, Jul 28, 2017 at 7:23 AM, Tom Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
> On Thu, Jul 27, 2017 at 03:39:58PM -0700, Laura Abbott wrote:
>> I don't know the intricacies of the Mustang hardware but external
>> aborts have been a symptom of missing clocks on other hardware.
>
> you are right, it's a missing clock. For SGMII ports the driver
> doesn't really use the clock source other then doing the one
> devm_clk_get(), but this is enough to get the clock going.
>
> Below patch fixes the crash and brings back the second SGMII port
> (even without a clock source in device tree, I guess it uses clock
> from first port). From a correctness standpoint there should probably
> be a clk_prepare_enable(), if port is SGMII.
>
> Could you test the patch ? If it works, I'll submit it.
>
> Thomas.
>
> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
> index 86058a9f3417..1d307f2def2d 100644
> --- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
> +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
> @@ -1785,9 +1785,9 @@ static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
>
> xgene_enet_gpiod_get(pdata);
>
> - if (pdata->phy_mode != PHY_INTERFACE_MODE_SGMII) {
> - pdata->clk = devm_clk_get(&pdev->dev, NULL);
> - if (IS_ERR(pdata->clk)) {
> + pdata->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(pdata->clk)) {
> + if (pdata->phy_mode != PHY_INTERFACE_MODE_SGMII) {
> /* Abort if the clock is defined but couldn't be
> * retrived. Always abort if the clock is missing on
> * DT system as the driver can't cope with this case.
>
> --
Thanks for the patch.
Acked-by: Iyappan Subramanian <isubramanian@apm.com>
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea. [ RFC1925, 2.3 ]
^ permalink raw reply
* Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings
From: Oliver Hartkopp @ 2017-07-28 18:33 UTC (permalink / raw)
To: Franklin S Cooper Jr, Andrew Lunn, linux-kernel, devicetree,
netdev, linux-can, wg, mkl, robh+dt, quentin.schulz,
sergei.shtylyov, dev.kurt
In-Reply-To: <20170728130231.GA10680@airbook.vandijck-laurijssen.be>
Hi Kurt,
On 07/28/2017 03:02 PM, Kurt Van Dijck wrote:
>>> The word 'max-arbitration-bitrate' makes the difference very clear.
>>
>> I think you are mixing up ISO layer 1 and ISO layer 2.
>
> In order to provide higher data throughput without putting extra limits
> on transceiver & wire, the requirement for the round-trip delay to be
> within 1 bittime has been eliminated, but only for the data phase when
> arbitration is over.
> So layer 2 (CAN FD) has been adapted to circumvent the layer 1
> (transceiver + wire) limitations.
>
> In fact, the round-trip delay requirement never actually did matter for
> plain CAN during data bits either. CAN FD just makes use of that,
> but is therefore incompatible on the wire.
>
> I forgot the precise wording, but this is the principle that Bosch
> explained on the CAN conference in Nurnberg several years ago, or at
> least this is how I remembered it :-)
I just checked an example for a CAN FD qualified transceiver
http://www.nxp.com/products/automotive-products/energy-power-management/can-transceivers/high-speed-can-transceiver-with-standby-mode:TJA1044
where it states:
The TJA1044T is specified for data rates up to 1 Mbit/s. Pending the
release of ISO11898-2:2016 including CAN FD and SAE-J2284-4/5,
additional timing parameters defining loop delay symmetry are specified
for the TJA1044GT and TJA1044GTK. This implementation enables reliable
communication in the CAN FD fast phase at data rates up to 5 Mbit/s.
and
TJA1044GT/TJA1044GTK
- Timing guaranteed for data rates up to 5 Mbit/s
- Improved TXD to RXD propagation delay of 210 ns
> I haven't followed the developments of transceivers, but with the above
> principle in mind, it's obvious that any transceiver allows higher
> bitrates during the data segment because the TX-to-RX line delay must
> not scale with the bitrate.
> In reality, maybe not all transceivers will mention this in their
> datasheet.
>
> So whether you call it 'max-arbitration-bitrate' & 'max-data-bitrate'
> or 'max-bitrate' & 'max-data-bitrate' does not really matter (I prefer
> 1st) but you will one day need 2 bitrates.
The question to me is whether it is right option to specify two bitrates
OR to specify one maximum bitrate and provide a property that a CAN FD
capable propagation delay is available.
E.g.
max-bitrate
max-data-bitrate
or
max-bitrate
canfd-capable // CAN FD capable propagation delay available
I assume the optimized propagation delay is 'always on' as the
transceiver is not able to detect which kind of bits it is processing.
That's why I think providing two bitrates leads to a wrong view on the
transceiver.
Regards,
Oliver
^ permalink raw reply
* Re: refcount_t + (resend to wider audience)
From: Mark Salyzyn @ 2017-07-28 18:07 UTC (permalink / raw)
To: Andrew Lunn; +Cc: davem, netdev, stable
In-Reply-To: <20170728174117.GL2132@lunn.ch>
On 07/28/2017 10:41 AM, Andrew Lunn wrote:
> On Fri, Jul 28, 2017 at 10:15:23AM -0700, Mark Salyzyn wrote:
>> (Resend to wider audience to comply with
>> Documentation/networking/netdev-FAQ.txt)
>>
>> Please apply/backport the following upstream feature and followup
>> grouped fixes patches to the stable trees (expect included in at least
>> 3.10.y, 3.18.y, 4.4.y and 4.9.y):
> .. _stable_kernel_rules:
>
> Hi Mark
>
> Everything you ever wanted to know about Linux -stable releases
> ===============================================================
>
> Rules on what kind of patches are accepted, and which ones are not, into the
> "-stable" tree:
>
> - It must be obviously correct and tested.
> - It cannot be bigger than 100 lines, with context.
>
> The first patch you list is 342 lines. The second one is 634.
>
> Please could you read the rules and then provide some justification
> for ignoring many of the rules.
>
> Andrew
The first four patches add a new dependent upstream API and type,
refcount_t. New APIs will notoriously cause a large number of lines to
be adjusted. They are complete (ToT will/should match stable),
orthogonal, and without CONFIG_REFCOUNT_FULL completely inert in all
places where atomic_t reference counters used, and are replaced with
refcount_t in the followup patches that take advantage of this new type.
The first four do _nothing_ at all to the kernel as-is, but represent a
dependency for the following changes.
The remaining patches (for the most part) take advantage of this new API
to mostly fix, or report/warn when they can not, Use-After-Free (KASAN)
bugs which can lead to root attack exploits. atomic_t are subject to
unbounded attacks, refcount_t are relatively immune to unbounded
attacks. It is admittedly not a complete fix, but greatly reduce the
chances of the security issues. The recommendation is to turn on
CONFIG_REFCOUNT_FULL, but that is a decision to balance between security
and performance.
For any platform that requires the latest security updates, refcount_t
is going to be a requirement. I urge you to overlook the first four
patch sizes because of their status as an orthogonal type and API,
necessary dependency for security improvements.
Sincerely -- Mark Salyzyn
^ permalink raw reply
* Re: [v4.12 regression] netns: NULL deref in fib_sync_down_dev()
From: Michał Mirosław @ 2017-07-28 18:04 UTC (permalink / raw)
To: Ido Schimmel; +Cc: Cong Wang, Linux Kernel Network Developers
In-Reply-To: <20170728173602.GA30482@splinter>
On Fri, Jul 28, 2017 at 08:36:02PM +0300, Ido Schimmel wrote:
> On Fri, Jul 28, 2017 at 10:28:16AM -0700, Cong Wang wrote:
> > On Fri, Jul 28, 2017 at 9:43 AM, Ido Schimmel <idosch@idosch.org> wrote:
> > > On Fri, Jul 28, 2017 at 06:00:47PM +0200, Michał Mirosław wrote:
> > >> Dear NetDevs,
> > >>
> > >> Before I go to bisecting, have you seen a following NULL dereference,
> > >> yet? Where should I start looking? It is triggered by deleting netns
> > >> (cut-down script attached - triggers every time). This was working
> > >> correctly under v4.11.x.
> > > Thanks for the report. I just reproduced this on my system. I believe
> > > the problem is a missing NULL check for 'in_dev' in
> > > call_fib_nh_notifiers(). I'll test a fix.
> > But your commit 982acb97560c8118c2109504a22b0d78a580547d
> > is merged in v4.11-rc1. How could 4.11.x work correctly?
> It doesn't. I just reproduced this on v4.11.
Thanks for looking into this. I was sure that I ran v4.11.7 last time,
but it turns out I worked on this earlier than that. I'll be glad to
test patches for this issue when you have it.
Best Regards,
Michał Mirosław
^ 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