* Re: [PATCH] serial: add a new helper function
From: Alan Cox @ 2012-08-19 15:46 UTC (permalink / raw)
To: Greg KH; +Cc: Huang Shijie, alan, jirislaby, linux-kernel, linux-serial, netdev
In-Reply-To: <20120819064429.GA3252@kroah.com>
On Sat, 18 Aug 2012 23:44:29 -0700
Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sun, Aug 19, 2012 at 02:27:12PM -0400, Huang Shijie wrote:
> > --- a/include/linux/tty.h
> > +++ b/include/linux/tty.h
> > @@ -43,6 +43,7 @@
> > #include <linux/tty_driver.h>
> > #include <linux/tty_ldisc.h>
> > #include <linux/mutex.h>
> > +#include <linux/serial.h>
> >
> >
> >
> > @@ -513,6 +514,12 @@ static inline struct tty_port *tty_port_get(struct tty_port *port)
> > return port;
> > }
> >
> > +/* If the cts flow control is enabled, return true. */
> > +static inline bool tty_port_cts_enabled(struct tty_port *port)
> > +{
> > + return port->flags & ASYNC_CTS_FLOW;
> > +}
> > +
>
> The fact that you have to add serial.h to this file kind of implies that
> this function shouldn't be here, right?
>
> How about serial.h instead? Not all tty drivers are serial drivers :)
tty_port is tty generic so possibly if there is a generic helper the
flags and helper should likewise be this way.
As it stands at the moment ASYNC_CTS_FLOW is a convention a few drivers
use. So calling it tty_port_xxx is going to misleading.
Alan
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Lin Ming @ 2012-08-19 15:05 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <1345387532.5158.222.camel@edumazet-glaptop>
On Sun, Aug 19, 2012 at 10:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2012-08-19 at 22:15 +0800, Lin Ming wrote:
>
>> Will it still has problem if code goes here without sock_hold(sk)?
>
> Not sure of what you mean.
See my comments in the function.
Is that a potential problem?
static void tcp_write_timer(unsigned long data)
{
struct sock *sk = (struct sock *)data;
bh_lock_sock(sk);
if (!sock_owned_by_user(sk)) {
tcp_write_timer_handler(sk);
//if we arrive here with sk_refcnt=1
//then below sock_put(sk) will also free the sock.
} else {
/* deleguate our work to tcp_release_cb() */
set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
}
bh_unlock_sock(sk);
sock_put(sk);
}
>
> At the time tcp_write_timer() runs, we own one reference on the socket.
> (this reference was taken in sk_reset_timer())
>
> On old kernels, if we found the socket locked by the user, we used to
> rearm the timer for a 50ms delay (and thus did sock_hold() again)
>
> Another way to avoid the bug would to make sure sk_reset_timer()
> increases refcount _before_ setting the timer, but its adding one atomic
> in fast path...
I think this is safer than your previous fix.
Thanks.
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 8f67ced..d1745b7 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2114,8 +2114,9 @@ EXPORT_SYMBOL(sk_send_sigurg);
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
> - if (!mod_timer(timer, expires))
> - sock_hold(sk);
> + sock_hold(sk);
> + if (mod_timer(timer, expires))
> + __sock_put(sk);
> }
> EXPORT_SYMBOL(sk_reset_timer);
>
>
>
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Eric Dumazet @ 2012-08-19 14:45 UTC (permalink / raw)
To: Lin Ming; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <CAF1ivSaU_ur9BkKG3eAsNw7d+ND5xgYbPKutemVqNec3X9m4SA@mail.gmail.com>
On Sun, 2012-08-19 at 22:15 +0800, Lin Ming wrote:
> Will it still has problem if code goes here without sock_hold(sk)?
Not sure of what you mean.
At the time tcp_write_timer() runs, we own one reference on the socket.
(this reference was taken in sk_reset_timer())
On old kernels, if we found the socket locked by the user, we used to
rearm the timer for a 50ms delay (and thus did sock_hold() again)
Another way to avoid the bug would to make sure sk_reset_timer()
increases refcount _before_ setting the timer, but its adding one atomic
in fast path...
diff --git a/net/core/sock.c b/net/core/sock.c
index 8f67ced..d1745b7 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2114,8 +2114,9 @@ EXPORT_SYMBOL(sk_send_sigurg);
void sk_reset_timer(struct sock *sk, struct timer_list* timer,
unsigned long expires)
{
- if (!mod_timer(timer, expires))
- sock_hold(sk);
+ sock_hold(sk);
+ if (mod_timer(timer, expires))
+ __sock_put(sk);
}
EXPORT_SYMBOL(sk_reset_timer);
^ permalink raw reply related
* Re: [PATCH v2 01/16] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-08-19 14:16 UTC (permalink / raw)
To: Sasha Levin
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
lw-BthXqXjhjHXQFUHtdCDX3A, teigland-H+wXaHxf7aLQT0dZR+AlfA,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, tj-DgEjT+Ai2ygdnm+yROfE0A,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20120819131637.GA8272@Krystal>
* Mathieu Desnoyers (mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org) wrote:
> * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
[...]
> > +/**
> > + * hash_for_each_possible - iterate over all possible objects for a given key
> > + * @name: hashtable to iterate
> > + * @obj: the type * to use as a loop cursor for each bucket
> > + * @bits: bit count of hashing function of the hashtable
> > + * @node: the &struct list_head to use as a loop cursor for each bucket
> > + * @member: the name of the hlist_node within the struct
> > + * @key: the key of the objects to iterate over
> > + */
> > +#define hash_for_each_possible_size(name, obj, bits, node, member, key) \
> > + hlist_for_each_entry(obj, node, &name[hash_min(key, bits)], member)
>
> Second point: "for_each_possible" does not express the iteration scope.
> Citing WordNet: "possible adj 1: capable of happening or existing;" --
> which has nothing to do with iteration on duplicate keys within a hash
> table.
>
> I would recommend to rename "possible" to "duplicate", e.g.:
>
> hash_for_each_duplicate()
>
> which clearly says what is the scope of this iteration: duplicate keys.
OK, about this part: I now see that you iterate over all objects within
the same hash chain. I guess the description "iterate over all possible
objects for a given key" is misleading: it's not all objects with a
given key, but rather all objects hashing to the same bucket.
I understand that you don't want to build knowledge of the key
comparison function in the iterator (which makes sense for a simple hash
table).
By the way, the comment "@obj: the type * to use as a loop cursor for
each bucket" is also misleading: it's a loop cursor for each entry,
since you iterate on all nodes within single bucket. Same for "@node:
the &struct list_head to use as a loop cursor for each bucket".
So with these documentation changes applied, hash_for_each_possible
starts to make more sense, because it refers to entries that can
_possibly_ be a match (or not). Other options would be
hash_chain_for_each() or hash_bucket_for_each().
Thanks,
Mathieu
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Lin Ming @ 2012-08-19 14:15 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <1345380682.5158.201.camel@edumazet-glaptop>
On Sun, Aug 19, 2012 at 8:51 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
<snip...>
> Hi Fengguang, thanks for this report.
>
> Hmm, this looks like sk_reset_timer() is called on a socket, and timer
> triggers _before_ the sock_hold()
>
> So the timer handler decrements sk_refcnt to 0 and calls sk_free()
>
> Its probably a bug introduced (or uncovered) by commit 6f458dfb40 (tcp:
> improve latencies of timer triggered events)
>
> I always found sk_reset_timer() a bit racy...
>
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
> if (!mod_timer(timer, expires))
> sock_hold(sk); // MIGHT BE TOO LATE
> }
>
> Following should be safer...
>
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
> sock_hold(sk);
> if (mod_timer(timer, expires))
> sock_put(sk);
> }
>
> Could you test following patch ?
>
> By the way, there is a typo in tcp_delack_timer()
> (should use TCP_DELACK_TIMER_DEFERRED instead of
> TCP_WRITE_TIMER_DEFERRED)
>
>
> Thanks !
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 7678237..6278a11 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -417,10 +417,12 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
>
> if (code == ICMP_FRAG_NEEDED) { /* PMTU discovery (RFC1191) */
> tp->mtu_info = info;
> - if (!sock_owned_by_user(sk))
> + if (!sock_owned_by_user(sk)) {
> tcp_v4_mtu_reduced(sk);
> - else
> - set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags);
> + } else {
> + if (!test_and_set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags))
> + sock_hold(sk);
> + }
> goto out;
> }
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 20dfd89..d046326 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -910,14 +910,18 @@ void tcp_release_cb(struct sock *sk)
> if (flags & (1UL << TCP_TSQ_DEFERRED))
> tcp_tsq_handler(sk);
>
> - if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED))
> + if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED)) {
> tcp_write_timer_handler(sk);
> -
> - if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED))
> + __sock_put(sk);
> + }
> + if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED)) {
> tcp_delack_timer_handler(sk);
> -
> - if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED))
> + __sock_put(sk);
> + }
> + if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED)) {
> sk->sk_prot->mtu_reduced(sk);
> + __sock_put(sk);
> + }
> }
> EXPORT_SYMBOL(tcp_release_cb);
>
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 6df36ad..b774a03 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -252,7 +252,8 @@ static void tcp_delack_timer(unsigned long data)
> inet_csk(sk)->icsk_ack.blocked = 1;
> NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_DELAYEDACKLOCKED);
> /* deleguate our work to tcp_release_cb() */
> - set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
> + if (!test_and_set_bit(TCP_DELACK_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags))
> + sock_hold(sk);
> }
> bh_unlock_sock(sk);
> sock_put(sk);
> @@ -481,7 +482,8 @@ static void tcp_write_timer(unsigned long data)
> tcp_write_timer_handler(sk);
Will it still has problem if code goes here without sock_hold(sk)?
> } else {
> /* deleguate our work to tcp_release_cb() */
> - set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
> + if (!test_and_set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags))
> + sock_hold(sk);
> }
> bh_unlock_sock(sk);
> sock_put(sk);
>
^ permalink raw reply
* Re: [RFC] Interface for TCP Metrics
From: Eric Dumazet @ 2012-08-19 13:50 UTC (permalink / raw)
To: Julian Anastasov; +Cc: netdev
In-Reply-To: <alpine.LFD.2.00.1208191402560.1893@ja.ssi.bg>
On Sun, 2012-08-19 at 14:42 +0300, Julian Anastasov wrote:
> Hello,
>
> Once DaveM mentioned that TCP Metrics need their
> own interface. I'm planning to implement such interface
> but lets first decide how it should look. After little
> research, here is my plan:
>
> - will use genl with TCP_METRICS_GENL_NAME "tcp_metrics",
> TCP_METRICS_GENL_VERSION 0x01
>
> - provide dumpit method and one cmd to read metrics by exact addr,
> will use TCP_METRICS_CMD_{GET,...} and TCP_METRICS_ATTR_xxx in
> new file include/linux/tcp_metrics.h
>
> - Is command to delete cached entry needed? Delete will need
> new rcu_head. Useful to flush the cache or to delete entries
> with filter.
>
> - without support for delete cmd, may be we can add command to
> reset entry with default values from dst?
>
> - Where to put the new netlink code?
> tcp_metrics_netlink.c
> tcp_metrics_nl.c
> or just in current tcp_metrics.c ?
>
> - will provide support for ip tool:
>
> ip tcpm[etrics] <cmd> ...
> ip -6 tcpm[etrics] get <addr> => TCP_METRICS_CMD_GET for 1 entry
> ip tcpm[etrics] list => dumpit
>
> - any wishes how to look the output? How to select specific
> values/metrics in output? For example, list only specified
> named values: ip tcpm list [-o] rtt
>
> - command to modify specific metric for addr, by name? Only
> for tcpm_vals? If not locked?
>
> Do we need modify/delete/reset support or just read
> support is enough? Comments?
>
> Regards
One way to delete one or all entries would be good, for being able to
reproduce some tests from a known state.
^ permalink raw reply
* [PATCH net-next] ipv6: gre: fix ip6gre_err()
From: Eric Dumazet @ 2012-08-19 13:47 UTC (permalink / raw)
To: Dmitry Kozlov; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
ip6gre_err() miscomputes grehlen (sizeof(ipv6h) is 4 or 8,
not 40 as expected), and should take into account 'offset' parameter.
Also uses pskb_may_pull() to cope with some fragged skbs
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Dmitry Kozlov <xeb@mail.ru>
---
net/ipv6/ip6_gre.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index a84ad5d..424d11a 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -415,8 +415,8 @@ static void ip6gre_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
u8 type, u8 code, int offset, __be32 info)
{
const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)skb->data;
- __be16 *p = (__be16 *)(ipv6h + 1);
- int grehlen = sizeof(ipv6h) + 4;
+ __be16 *p = (__be16 *)(skb->data + offset);
+ int grehlen = offset + 4;
struct ip6_tnl *t;
__be16 flags;
@@ -432,8 +432,10 @@ static void ip6gre_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
}
/* If only 8 bytes returned, keyed message will be dropped here */
- if (skb_headlen(skb) < grehlen)
+ if (!pskb_may_pull(skb, grehlen))
return;
+ ipv6h = (const struct ipv6hdr *)skb->data;
+ p = (__be16 *)(skb->data + offset);
rcu_read_lock();
^ permalink raw reply related
* Re: [PATCH net v2] net: tcp: move sk_rx_dst_set call after tcp_create_openreq_child()
From: Eric Dumazet @ 2012-08-19 13:41 UTC (permalink / raw)
To: Neal Cardwell; +Cc: David Miller, netdev, Eric Dumazet
In-Reply-To: <1345383038-8330-1-git-send-email-ncardwell@google.com>
On Sun, 2012-08-19 at 09:30 -0400, Neal Cardwell wrote:
> This commit removes the sk_rx_dst_set calls from
> tcp_create_openreq_child(), because at that point the icsk_af_ops
> field of ipv6_mapped TCP sockets has not been set to its proper final
> value.
>
> Instead, to make sure we get the right sk_rx_dst_set variant
> appropriate for the address family of the new connection, we have
> tcp_v{4,6}_syn_recv_sock() directly call the appropriate function
> shortly after the call to tcp_create_openreq_child() returns.
>
> This also moves inet6_sk_rx_dst_set() to avoid a forward declaration
> with the new approach.
>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Reported-by: Artem Savkov <artem.savkov@gmail.com>
> Cc: Eric Dumazet <edumazet@google.com>
> ---
> net/ipv4/tcp_ipv4.c | 1 +
> net/ipv4/tcp_minisocks.c | 2 --
> net/ipv6/tcp_ipv6.c | 25 +++++++++++++------------
> 3 files changed, 14 insertions(+), 14 deletions(-)
Thanks Neal !
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* [PATCH net v2] net: tcp: move sk_rx_dst_set call after tcp_create_openreq_child()
From: Neal Cardwell @ 2012-08-19 13:30 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Eric Dumazet, Neal Cardwell
This commit removes the sk_rx_dst_set calls from
tcp_create_openreq_child(), because at that point the icsk_af_ops
field of ipv6_mapped TCP sockets has not been set to its proper final
value.
Instead, to make sure we get the right sk_rx_dst_set variant
appropriate for the address family of the new connection, we have
tcp_v{4,6}_syn_recv_sock() directly call the appropriate function
shortly after the call to tcp_create_openreq_child() returns.
This also moves inet6_sk_rx_dst_set() to avoid a forward declaration
with the new approach.
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Reported-by: Artem Savkov <artem.savkov@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_ipv4.c | 1 +
net/ipv4/tcp_minisocks.c | 2 --
net/ipv6/tcp_ipv6.c | 25 +++++++++++++------------
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 7678237..5bf2040 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1462,6 +1462,7 @@ struct sock *tcp_v4_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
goto exit_nonewsk;
newsk->sk_gso_type = SKB_GSO_TCPV4;
+ inet_sk_rx_dst_set(newsk, skb);
newtp = tcp_sk(newsk);
newinet = inet_sk(newsk);
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index d9c9dce..6ff7f10 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -387,8 +387,6 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
struct tcp_sock *oldtp = tcp_sk(sk);
struct tcp_cookie_values *oldcvp = oldtp->cookie_values;
- newicsk->icsk_af_ops->sk_rx_dst_set(newsk, skb);
-
/* TCP Cookie Transactions require space for the cookie pair,
* as it differs for each connection. There is no need to
* copy any s_data_payload stored at the original socket.
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index bb9ce2b..a3e60cc 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -94,6 +94,18 @@ static struct tcp_md5sig_key *tcp_v6_md5_do_lookup(struct sock *sk,
}
#endif
+static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
+{
+ struct dst_entry *dst = skb_dst(skb);
+ const struct rt6_info *rt = (const struct rt6_info *)dst;
+
+ dst_hold(dst);
+ sk->sk_rx_dst = dst;
+ inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
+ if (rt->rt6i_node)
+ inet6_sk(sk)->rx_dst_cookie = rt->rt6i_node->fn_sernum;
+}
+
static void tcp_v6_hash(struct sock *sk)
{
if (sk->sk_state != TCP_CLOSE) {
@@ -1270,6 +1282,7 @@ static struct sock * tcp_v6_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
newsk->sk_gso_type = SKB_GSO_TCPV6;
__ip6_dst_store(newsk, dst, NULL, NULL);
+ inet6_sk_rx_dst_set(newsk, skb);
newtcp6sk = (struct tcp6_sock *)newsk;
inet_sk(newsk)->pinet6 = &newtcp6sk->inet6;
@@ -1729,18 +1742,6 @@ static struct timewait_sock_ops tcp6_timewait_sock_ops = {
.twsk_destructor= tcp_twsk_destructor,
};
-static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
-{
- struct dst_entry *dst = skb_dst(skb);
- const struct rt6_info *rt = (const struct rt6_info *)dst;
-
- dst_hold(dst);
- sk->sk_rx_dst = dst;
- inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
- if (rt->rt6i_node)
- inet6_sk(sk)->rx_dst_cookie = rt->rt6i_node->fn_sernum;
-}
-
static const struct inet_connection_sock_af_ops ipv6_specific = {
.queue_xmit = inet6_csk_xmit,
.send_check = tcp_v6_send_check,
--
1.7.7.3
^ permalink raw reply related
* Re: [PATCH v2 01/16] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-08-19 13:16 UTC (permalink / raw)
To: Sasha Levin
Cc: torvalds, tj, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie, teigland,
Trond.Myklebust, bfields, fweisbec, jesse, venkat.x.venkatsubra,
ejt, snitzer, edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <1345337550-24304-2-git-send-email-levinsasha928@gmail.com>
* Sasha Levin (levinsasha928@gmail.com) wrote:
> This hashtable implementation is using hlist buckets to provide a simple
> hashtable to prevent it from getting reimplemented all over the kernel.
>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
> include/linux/hashtable.h | 284 +++++++++++++++++++++++++++++++++++++++++++++
[...]
Hi Sasha,
There are still a few API naming nits that I'd like to discuss:
> +
> +/**
> + * hash_for_each_size - iterate over a hashtable
> + * @name: hashtable to iterate
> + * @bits: bit count of hashing function of the hashtable
> + * @bkt: integer to use as bucket loop cursor
> + * @node: the &struct list_head to use as a loop cursor for each bucket
> + * @obj: the type * to use as a loop cursor for each bucket
> + * @member: the name of the hlist_node within the struct
> + */
> +#define hash_for_each_size(name, bits, bkt, node, obj, member) \
What is the meaning of "for each size" ?
By looking at the implementation, I see that it takes an extra "bits"
argument to specify the key width.
But in the other patches of this patchset, I cannot find a single user
of the "*_size" API. If you do not typically expect users to specify
this parameter by hand (thanks to use of HASH_BITS(name) in for_each
functions that do not take the bits parameter), I would recommend to
only expose hash_for_each() and similar defines, but not the *_size
variants.
So I recommend merging hash_for_each_size into hash_for_each (and
doing similarly for other *_size variants). On the plus side, it will
cut down the number of for_each macros from 12 down to 6, which is more
reasonable.
> + for (bkt = 0; bkt < HASH_SIZE(bits); bkt++) \
> + hlist_for_each_entry(obj, node, &name[bkt], member)
> +
> +/**
> + * hash_for_each - iterate over a hashtable
> + * @name: hashtable to iterate
> + * @bkt: integer to use as bucket loop cursor
> + * @node: the &struct list_head to use as a loop cursor for each bucket
> + * @obj: the type * to use as a loop cursor for each bucket
> + * @member: the name of the hlist_node within the struct
> + */
> +#define hash_for_each(name, bkt, node, obj, member) \
> + hash_for_each_size(name, HASH_BITS(name), bkt, node, obj, member)
> +
[...]
> +/**
> + * hash_for_each_possible - iterate over all possible objects for a given key
> + * @name: hashtable to iterate
> + * @obj: the type * to use as a loop cursor for each bucket
> + * @bits: bit count of hashing function of the hashtable
> + * @node: the &struct list_head to use as a loop cursor for each bucket
> + * @member: the name of the hlist_node within the struct
> + * @key: the key of the objects to iterate over
> + */
> +#define hash_for_each_possible_size(name, obj, bits, node, member, key) \
> + hlist_for_each_entry(obj, node, &name[hash_min(key, bits)], member)
Second point: "for_each_possible" does not express the iteration scope.
Citing WordNet: "possible adj 1: capable of happening or existing;" --
which has nothing to do with iteration on duplicate keys within a hash
table.
I would recommend to rename "possible" to "duplicate", e.g.:
hash_for_each_duplicate()
which clearly says what is the scope of this iteration: duplicate keys.
Thanks,
Mathieu
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Fengguang Wu @ 2012-08-19 13:04 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, networking, LKML
In-Reply-To: <1345380682.5158.201.camel@edumazet-glaptop>
> Could you test following patch ?
Yes, sure. Thanks for the quick fix! I should have reported it earlier
(so as to get it fixed earlier) once find the bisect will be time consuming..
Please be patient -- it will take 1-2 days to confirm the bug fix.
Thanks,
Fengguang
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Eric Dumazet @ 2012-08-19 12:51 UTC (permalink / raw)
To: Fengguang Wu; +Cc: David Miller, networking, LKML
In-Reply-To: <20120818021918.GA6499@localhost>
On Sat, 2012-08-18 at 10:19 +0800, Fengguang Wu wrote:
> Hi David,
>
> The bug should be introduced somewhere between 3.5 and 3.6-rc1.
>
> [ 2866.131281] IPv4: Attempt to release TCP socket in state 1 ffff880019ec0000
> [ 2866.131726]
> [ 2866.132188] =========================
> [ 2866.132281] [ BUG: held lock freed! ]
> [ 2866.132281] 3.6.0-rc1+ #622 Not tainted
> [ 2866.132281] -------------------------
> [ 2866.132281] kworker/0:1/652 is freeing memory ffff880019ec0000-ffff880019ec0a1f, with a lock still held there!
> [ 2866.132281] (sk_lock-AF_INET-RPC){+.+...}, at: [<ffffffff81903619>] tcp_sendmsg+0x29/0xcc6
> [ 2866.132281] 4 locks held by kworker/0:1/652:
> [ 2866.132281] #0: (rpciod){.+.+.+}, at: [<ffffffff81083567>] process_one_work+0x1de/0x47f
> [ 2866.132281] #1: ((&task->u.tk_work)){+.+.+.}, at: [<ffffffff81083567>] process_one_work+0x1de/0x47f
> [ 2866.132281] #2: (sk_lock-AF_INET-RPC){+.+...}, at: [<ffffffff81903619>] tcp_sendmsg+0x29/0xcc6
> [ 2866.132281] #3: (&icsk->icsk_retransmit_timer){+.-...}, at: [<ffffffff81078017>] run_timer_softirq+0x1ad/0x35f
> [ 2866.132281]
> [ 2866.132281] stack backtrace:
> [ 2866.132281] Pid: 652, comm: kworker/0:1 Not tainted 3.6.0-rc1+ #622
> [ 2866.132281] Call Trace:
> [ 2866.132281] <IRQ> [<ffffffff810bc527>] debug_check_no_locks_freed+0x112/0x159
> [ 2866.132281] [<ffffffff818a0839>] ? __sk_free+0xfd/0x114
> [ 2866.132281] [<ffffffff811549fa>] kmem_cache_free+0x6b/0x13a
> [ 2866.132281] [<ffffffff818a0839>] __sk_free+0xfd/0x114
> [ 2866.132281] [<ffffffff818a08c0>] sk_free+0x1c/0x1e
> [ 2866.132281] [<ffffffff81911e1c>] tcp_write_timer+0x51/0x56
> [ 2866.132281] [<ffffffff81078082>] run_timer_softirq+0x218/0x35f
> [ 2866.132281] [<ffffffff81078017>] ? run_timer_softirq+0x1ad/0x35f
> [ 2866.132281] [<ffffffff810f5831>] ? rb_commit+0x58/0x85
> [ 2866.132281] [<ffffffff81911dcb>] ? tcp_write_timer_handler+0x148/0x148
> [ 2866.132281] [<ffffffff81070bd6>] __do_softirq+0xcb/0x1f9
> [ 2866.132281] [<ffffffff81a0a00c>] ? _raw_spin_unlock+0x29/0x2e
> [ 2866.132281] [<ffffffff81a1227c>] call_softirq+0x1c/0x30
> [ 2866.132281] [<ffffffff81039f38>] do_softirq+0x4a/0xa6
> [ 2866.132281] [<ffffffff81070f2b>] irq_exit+0x51/0xad
> [ 2866.132281] [<ffffffff81a129cd>] do_IRQ+0x9d/0xb4
> [ 2866.132281] [<ffffffff81a0a3ef>] common_interrupt+0x6f/0x6f
> [ 2866.132281] <EOI> [<ffffffff8109d006>] ? sched_clock_cpu+0x58/0xd1
> [ 2866.132281] [<ffffffff81a0a172>] ? _raw_spin_unlock_irqrestore+0x4c/0x56
> [ 2866.132281] [<ffffffff81078692>] mod_timer+0x178/0x1a9
> [ 2866.132281] [<ffffffff818a00aa>] sk_reset_timer+0x19/0x26
> [ 2866.132281] [<ffffffff8190b2cc>] tcp_rearm_rto+0x99/0xa4
> [ 2866.132281] [<ffffffff8190dfba>] tcp_event_new_data_sent+0x6e/0x70
> [ 2866.132281] [<ffffffff8190f7ea>] tcp_write_xmit+0x7de/0x8e4
> [ 2866.132281] [<ffffffff818a565d>] ? __alloc_skb+0xa0/0x1a1
> [ 2866.132281] [<ffffffff8190f952>] __tcp_push_pending_frames+0x2e/0x8a
> [ 2866.132281] [<ffffffff81904122>] tcp_sendmsg+0xb32/0xcc6
> [ 2866.132281] [<ffffffff819229c2>] inet_sendmsg+0xaa/0xd5
> [ 2866.132281] [<ffffffff81922918>] ? inet_autobind+0x5f/0x5f
> [ 2866.132281] [<ffffffff810ee7f1>] ? trace_clock_local+0x9/0xb
> [ 2866.132281] [<ffffffff8189adab>] sock_sendmsg+0xa3/0xc4
> [ 2866.132281] [<ffffffff810f5de6>] ? rb_reserve_next_event+0x26f/0x2d5
> [ 2866.132281] [<ffffffff8103e6a9>] ? native_sched_clock+0x29/0x6f
> [ 2866.132281] [<ffffffff8103e6f8>] ? sched_clock+0x9/0xd
> [ 2866.132281] [<ffffffff810ee7f1>] ? trace_clock_local+0x9/0xb
> [ 2866.132281] [<ffffffff8189ae03>] kernel_sendmsg+0x37/0x43
> [ 2866.132281] [<ffffffff8199ce49>] xs_send_kvec+0x77/0x80
> [ 2866.132281] [<ffffffff8199cec1>] xs_sendpages+0x6f/0x1a0
> [ 2866.132281] [<ffffffff8107826d>] ? try_to_del_timer_sync+0x55/0x61
> [ 2866.132281] [<ffffffff8199d0d2>] xs_tcp_send_request+0x55/0xf1
> [ 2866.132281] [<ffffffff8199bb90>] xprt_transmit+0x89/0x1db
> [ 2866.132281] [<ffffffff81999bcd>] ? call_connect+0x3c/0x3c
> [ 2866.132281] [<ffffffff81999d92>] call_transmit+0x1c5/0x20e
> [ 2866.132281] [<ffffffff819a0d55>] __rpc_execute+0x6f/0x225
> [ 2866.132281] [<ffffffff81999bcd>] ? call_connect+0x3c/0x3c
> [ 2866.132281] [<ffffffff819a0f33>] rpc_async_schedule+0x28/0x34
> [ 2866.132281] [<ffffffff810835d6>] process_one_work+0x24d/0x47f
> [ 2866.132281] [<ffffffff81083567>] ? process_one_work+0x1de/0x47f
> [ 2866.132281] [<ffffffff819a0f0b>] ? __rpc_execute+0x225/0x225
> [ 2866.132281] [<ffffffff81083a6d>] worker_thread+0x236/0x317
> [ 2866.132281] [<ffffffff81083837>] ? process_scheduled_works+0x2f/0x2f
> [ 2866.132281] [<ffffffff8108b7b8>] kthread+0x9a/0xa2
> [ 2866.132281] [<ffffffff81a12184>] kernel_thread_helper+0x4/0x10
> [ 2866.132281] [<ffffffff81a0a4b0>] ? retint_restore_args+0x13/0x13
> [ 2866.132281] [<ffffffff8108b71e>] ? __init_kthread_worker+0x5a/0x5a
> [ 2866.132281] [<ffffffff81a12180>] ? gs_change+0x13/0x13
> [ 2866.308506] IPv4: Attempt to release TCP socket in state 1 ffff880019ec0000
> [ 2866.309689] =============================================================================
> [ 2866.310254] BUG TCP (Not tainted): Object already free
> [ 2866.310254] -----------------------------------------------------------------------------
> [ 2866.310254]
>
> This happens only in one of my test boxes and is rather hard to
> reproduce. I find that it could only happen once out of 100 boots,
> each boot test takes 1.5 hour. It's basically an NFSROOT system that
> runs a bunch of IO and MM tests.
>
> I've been running auto+manual bisects for one week and it has failed
> several times. Here is the log for last try. It reports 0bb4087 as
> the first bad commit, however the bug still shows up when reverting
> 0bb4087 on top of 3.6-rc2. So the 'good' commits in the below log may
> not really be good. The 'bad' commits, however, are bad for sure.
>
> wfg@bee ~/linux% git bisect log
> # bad: [0d7614f09c1ebdbaa1599a5aba7593f147bf96ee] Linux 3.6-rc1
> # good: [28a33cbc24e4256c143dce96c7d93bf423229f92] Linux 3.5
> git bisect start 'v3.6-rc1' 'v3.5' '--'
> # bad: [614a6d4341b3760ca98a1c2c09141b71db5d1e90] Merge branch 'for-3.6' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup
> git bisect bad 614a6d4341b3760ca98a1c2c09141b71db5d1e90
> # bad: [320f5ea0cedc08ef65d67e056bcb9d181386ef2c] genetlink: define lockdep_genl_is_held() when CONFIG_LOCKDEP
> git bisect bad 320f5ea0cedc08ef65d67e056bcb9d181386ef2c
> # skip: [dbfa600148a25903976910863c75dae185f8d187] cxgb3: set maximal number of default RSS queues
> git bisect skip dbfa600148a25903976910863c75dae185f8d187
> # bad: [b6dfd939fdc249fcf8cd7b8006f76239b33eb581] ixgbe: add support for new 82599 device
> git bisect bad b6dfd939fdc249fcf8cd7b8006f76239b33eb581
> # bad: [39cb681b3bb4da17e74d48e553d1bb9a1b759aa5] ixgbe: Fix handling of FDIR_HASH flag
> git bisect bad 39cb681b3bb4da17e74d48e553d1bb9a1b759aa5
> # bad: [9297127b9cdd8d30c829ef5fd28b7cc0323a7bcd] ixgbe: Change how we check for pre-existing and assigned VFs
> git bisect bad 9297127b9cdd8d30c829ef5fd28b7cc0323a7bcd
> # bad: [99d744875d01d57d832b8dbfc36d9a1990d503b8] ixgbe: Drop probe_vf and merge functionality into ixgbe_enable_sriov
> git bisect bad 99d744875d01d57d832b8dbfc36d9a1990d503b8
> # bad: [186e868786f97c8026f0a81400b451ace306b3a4] forcedeth: spin_unlock_irq in interrupt handler fix
> git bisect bad 186e868786f97c8026f0a81400b451ace306b3a4
> # good: [3e4b9459fb0e149c6b74c9e89399a8fc39a92b44] Merge tag 'md-3.5-fixes' of git://neil.brown.name/md
> git bisect good 3e4b9459fb0e149c6b74c9e89399a8fc39a92b44
> # good: [e8c7b335faca2cbce715da3b0e1663d75d422f5b] wlcore: increase command completion timeout
> git bisect good e8c7b335faca2cbce715da3b0e1663d75d422f5b
> # good: [1d248b1cf4e09dbec8cef5f7fbeda5874248bd09] net: Pass neighbours and dest address into NETEVENT_REDIRECT events.
> git bisect good 1d248b1cf4e09dbec8cef5f7fbeda5874248bd09
> # good: [73dea3983a9d2e413e1c065ed4e132a7d4127929] be2net: Add description about various RSS hash types
> git bisect good 73dea3983a9d2e413e1c065ed4e132a7d4127929
> # good: [8b0d2f9ed3d8e92feada7c5d70fa85be46e6f948] net: e100: ucode is optional in some cases
> git bisect good 8b0d2f9ed3d8e92feada7c5d70fa85be46e6f948
> # good: [36eb22e97a2b621fb707eead58ef915ab0f46e9e] libertas: firmware.c: remove duplicated include
> git bisect good 36eb22e97a2b621fb707eead58ef915ab0f46e9e
> # good: [5815d5e7aae3cc9c5e85af83094d4d6498c3f4fc] tcp: use hash_32() in tcp_metrics
> git bisect good 5815d5e7aae3cc9c5e85af83094d4d6498c3f4fc
> # good: [e4bce0f288bafd8505ba5ce9c5284a4478f1b725] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net-next
> git bisect good e4bce0f288bafd8505ba5ce9c5284a4478f1b725
> # good: [a1b5d0dd28e9cb4fe42ad2df4ebbe5cce96866d7] openvswitch: Check gso_type for correct sk_buff in queue_gso_packets().
> git bisect good a1b5d0dd28e9cb4fe42ad2df4ebbe5cce96866d7
> # bad: [0bb4087] ipv4: Fix neigh lookup keying over loopback/point-to-point devices.
> git bisect bad 0bb4087
> # good: [b09e786bd1dd66418b69348cb110f3a64764626a] tun: fix a crash bug and a memory leak
> git bisect good b09e786bd1dd66418b69348cb110f3a64764626a
> # good: [fa0afcd10951afad2022dda09777d2bf70cdab3d] atl1c: fix issue of io access mode for AR8152 v2.1
> git bisect good fa0afcd10951afad2022dda09777d2bf70cdab3d
>
> Thanks,
> Fengguang
Hi Fengguang, thanks for this report.
Hmm, this looks like sk_reset_timer() is called on a socket, and timer
triggers _before_ the sock_hold()
So the timer handler decrements sk_refcnt to 0 and calls sk_free()
Its probably a bug introduced (or uncovered) by commit 6f458dfb40 (tcp:
improve latencies of timer triggered events)
I always found sk_reset_timer() a bit racy...
void sk_reset_timer(struct sock *sk, struct timer_list* timer,
unsigned long expires)
{
if (!mod_timer(timer, expires))
sock_hold(sk); // MIGHT BE TOO LATE
}
Following should be safer...
void sk_reset_timer(struct sock *sk, struct timer_list* timer,
unsigned long expires)
{
sock_hold(sk);
if (mod_timer(timer, expires))
sock_put(sk);
}
Could you test following patch ?
By the way, there is a typo in tcp_delack_timer()
(should use TCP_DELACK_TIMER_DEFERRED instead of
TCP_WRITE_TIMER_DEFERRED)
Thanks !
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 7678237..6278a11 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -417,10 +417,12 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
if (code == ICMP_FRAG_NEEDED) { /* PMTU discovery (RFC1191) */
tp->mtu_info = info;
- if (!sock_owned_by_user(sk))
+ if (!sock_owned_by_user(sk)) {
tcp_v4_mtu_reduced(sk);
- else
- set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags);
+ } else {
+ if (!test_and_set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags))
+ sock_hold(sk);
+ }
goto out;
}
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 20dfd89..d046326 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -910,14 +910,18 @@ void tcp_release_cb(struct sock *sk)
if (flags & (1UL << TCP_TSQ_DEFERRED))
tcp_tsq_handler(sk);
- if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED))
+ if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED)) {
tcp_write_timer_handler(sk);
-
- if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED))
+ __sock_put(sk);
+ }
+ if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED)) {
tcp_delack_timer_handler(sk);
-
- if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED))
+ __sock_put(sk);
+ }
+ if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED)) {
sk->sk_prot->mtu_reduced(sk);
+ __sock_put(sk);
+ }
}
EXPORT_SYMBOL(tcp_release_cb);
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 6df36ad..b774a03 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -252,7 +252,8 @@ static void tcp_delack_timer(unsigned long data)
inet_csk(sk)->icsk_ack.blocked = 1;
NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_DELAYEDACKLOCKED);
/* deleguate our work to tcp_release_cb() */
- set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
+ if (!test_and_set_bit(TCP_DELACK_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags))
+ sock_hold(sk);
}
bh_unlock_sock(sk);
sock_put(sk);
@@ -481,7 +482,8 @@ static void tcp_write_timer(unsigned long data)
tcp_write_timer_handler(sk);
} else {
/* deleguate our work to tcp_release_cb() */
- set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags);
+ if (!test_and_set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags))
+ sock_hold(sk);
}
bh_unlock_sock(sk);
sock_put(sk);
^ permalink raw reply related
* $650,500.00 USD
From: United Nations Trust Funds @ 2012-08-19 11:47 UTC (permalink / raw)
To: Recipients
Reply to claim.
^ permalink raw reply
* $650,500.00 USD
From: United Nations Trust Funds @ 2012-08-19 11:42 UTC (permalink / raw)
To: Recipients
Reply to claim.
^ permalink raw reply
* [RFC] Interface for TCP Metrics
From: Julian Anastasov @ 2012-08-19 11:42 UTC (permalink / raw)
To: netdev
Hello,
Once DaveM mentioned that TCP Metrics need their
own interface. I'm planning to implement such interface
but lets first decide how it should look. After little
research, here is my plan:
- will use genl with TCP_METRICS_GENL_NAME "tcp_metrics",
TCP_METRICS_GENL_VERSION 0x01
- provide dumpit method and one cmd to read metrics by exact addr,
will use TCP_METRICS_CMD_{GET,...} and TCP_METRICS_ATTR_xxx in
new file include/linux/tcp_metrics.h
- Is command to delete cached entry needed? Delete will need
new rcu_head. Useful to flush the cache or to delete entries
with filter.
- without support for delete cmd, may be we can add command to
reset entry with default values from dst?
- Where to put the new netlink code?
tcp_metrics_netlink.c
tcp_metrics_nl.c
or just in current tcp_metrics.c ?
- will provide support for ip tool:
ip tcpm[etrics] <cmd> ...
ip -6 tcpm[etrics] get <addr> => TCP_METRICS_CMD_GET for 1 entry
ip tcpm[etrics] list => dumpit
- any wishes how to look the output? How to select specific
values/metrics in output? For example, list only specified
named values: ip tcpm list [-o] rtt
- command to modify specific metric for addr, by name? Only
for tcpm_vals? If not locked?
Do we need modify/delete/reset support or just read
support is enough? Comments?
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* $650,500.00 USD
From: United Nations Trust Funds @ 2012-08-19 11:15 UTC (permalink / raw)
To: Recipients
Reply to claim.
^ permalink raw reply
* Re: network packet corruption in v3.6.0-rc1 (and also in v3.5)
From: Eric Dumazet @ 2012-08-19 11:02 UTC (permalink / raw)
To: Andrew Worsley; +Cc: David S. Miller, Alexey Kuznetsov, James Morris, netdev
In-Reply-To: <CA+Y=x3=jSrEaYdnaAF75p0utQq02MhoSh4SkshRVSc7JjjVPOw@mail.gmail.com>
On Sat, 2012-08-18 at 16:40 +1000, Andrew Worsley wrote:
> Some times I get a burst of errors and dropped tcp connections with
> this warning. I had a similar issue with v3.5 (in fact v3.5 was
> unusable for https connections which would give bad MAC code messages
> at the application level)..
>
> The issue seems better under v3.6-rc1 where as it was very bad under
> v3.5. Let me know if there is something I can do to avoid this
> problem. I am using the USB r8712u wireless NIC if that is relevant as
> I haven't been able to get the native wireless working under the MAC
> book yet (requires proprietary firmware extraction which I am not
> familiar with)
>
> Much obliged if there is anything I can do to avoid it as it is rather
> annoying. Doesn't happen at all with my Debian wheezy 3.2.0-2-amd64
> kernel.
OK, this seems to be related to r8712u, still in staging tree.
At first glance, its using a buggy skb_clone(), or kind of a 'reuse skb'
trick I have no time to investigate.
I suspect that some memory area are overwritten by this driver, and this
was not noticed with older kernels. With new kernels, its triggering a
WARN_ON() in tcp stack.
Any chance you can try to reproduce the bug with another adapter ?
^ permalink raw reply
* Re: [PATCH] net: tcp: ipv6_mapped needs sk_rx_dst_set method
From: Eric Dumazet @ 2012-08-19 10:48 UTC (permalink / raw)
To: Artem Savkov; +Cc: David Miller, akpm, netdev
In-Reply-To: <20120818130642.GA10680@thinkpad.lan>
On Sat, 2012-08-18 at 17:06 +0400, Artem Savkov wrote:
> It doesn't seem to fix the problem with mapped ipv6 completely.
> I'm still hitting a NULL pointer dereference with mapped ipv6:
Thanks Artem for the report, I see Neal took care of the problem.
^ permalink raw reply
* Re: [PATCH] net: tcp: move sk_rx_dst_set call after tcp_create_openreq_child()
From: Eric Dumazet @ 2012-08-19 10:45 UTC (permalink / raw)
To: Neal Cardwell; +Cc: David Miller, netdev, Eric Dumazet
In-Reply-To: <1345348619-19327-1-git-send-email-ncardwell@google.com>
On Sat, 2012-08-18 at 23:56 -0400, Neal Cardwell wrote:
> This commit removes the sk_rx_dst_set calls from
> tcp_create_openreq_child(), because at that point the icsk_af_ops
> field of ipv6_mapped TCP sockets has not been set to its proper final
> value.
>
> Instead, to make sure we get the right sk_rx_dst_set variant
> appropriate for the address family of the new connection, we have
> tcp_v{4,6}_syn_recv_sock() directly call the appropriate function
> shortly after the call to tcp_create_openreq_child() returns.
>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Reported-by: Artem Savkov <artem.savkov@gmail.com>
> Cc: Eric Dumazet <edumazet@google.com>
> ---
> * copy any s_data_payload stored at the original socket.
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index bb9ce2b..492c2d9 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -81,6 +81,8 @@ static void __tcp_v6_send_check(struct sk_buff *skb,
> const struct in6_addr *saddr,
> const struct in6_addr *daddr);
>
> +static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb);
> +
Hi Neal, thanks for taking care of this.
Could you move inet6_sk_rx_dst_set() definition to avoid this forward ?
^ permalink raw reply
* [PATCH net-next] xfrm: fix RCU bugs
From: Eric Dumazet @ 2012-08-19 10:31 UTC (permalink / raw)
To: David Miller; +Cc: netdev, fan.du, fengguang.wu, Priyanka Jain
From: Eric Dumazet <edumazet@google.com>
This patch reverts commit 56892261ed1a (xfrm: Use rcu_dereference_bh to
deference pointer protected by rcu_read_lock_bh), and fixes bugs
introduced in commit 418a99ac6ad ( Replace rwlock on xfrm_policy_afinfo
with rcu )
1) We properly use RCU variant in this file, not a mix of RCU/RCU_BH
2) We must defer some writes after the synchronize_rcu() call or a reader
can crash dereferencing NULL pointer.
3) Now we use the xfrm_policy_afinfo_lock spinlock only from process
context, we no longer need to block BH in xfrm_policy_register_afinfo()
and xfrm_policy_unregister_afinfo()
4) Can use RCU_INIT_POINTER() instead of rcu_assign_pointer() in
xfrm_policy_unregister_afinfo()
5) Remove a forward inline declaration (xfrm_policy_put_afinfo()),
and also move xfrm_policy_get_afinfo() declaration.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Fan Du <fan.du@windriver.com>
Cc: Priyanka Jain <Priyanka.Jain@freescale.com>
---
net/xfrm/xfrm_policy.c | 76 ++++++++++++++++++++-------------------
1 file changed, 39 insertions(+), 37 deletions(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 6405764..e52f50f 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -48,8 +48,6 @@ static struct xfrm_policy_afinfo __rcu *xfrm_policy_afinfo[NPROTO]
static struct kmem_cache *xfrm_dst_cache __read_mostly;
-static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family);
-static inline void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo);
static void xfrm_init_pmtu(struct dst_entry *dst);
static int stale_bundle(struct dst_entry *dst);
static int xfrm_bundle_ok(struct xfrm_dst *xdst);
@@ -96,6 +94,24 @@ bool xfrm_selector_match(const struct xfrm_selector *sel, const struct flowi *fl
return false;
}
+static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
+{
+ struct xfrm_policy_afinfo *afinfo;
+
+ if (unlikely(family >= NPROTO))
+ return NULL;
+ rcu_read_lock();
+ afinfo = rcu_dereference(xfrm_policy_afinfo[family]);
+ if (unlikely(!afinfo))
+ rcu_read_unlock();
+ return afinfo;
+}
+
+static void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo)
+{
+ rcu_read_unlock();
+}
+
static inline struct dst_entry *__xfrm_dst_lookup(struct net *net, int tos,
const xfrm_address_t *saddr,
const xfrm_address_t *daddr,
@@ -2419,7 +2435,7 @@ int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo)
return -EINVAL;
if (unlikely(afinfo->family >= NPROTO))
return -EAFNOSUPPORT;
- spin_lock_bh(&xfrm_policy_afinfo_lock);
+ spin_lock(&xfrm_policy_afinfo_lock);
if (unlikely(xfrm_policy_afinfo[afinfo->family] != NULL))
err = -ENOBUFS;
else {
@@ -2442,7 +2458,7 @@ int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo)
afinfo->garbage_collect = xfrm_garbage_collect_deferred;
rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family], afinfo);
}
- spin_unlock_bh(&xfrm_policy_afinfo_lock);
+ spin_unlock(&xfrm_policy_afinfo_lock);
rtnl_lock();
for_each_net(net) {
@@ -2475,23 +2491,26 @@ int xfrm_policy_unregister_afinfo(struct xfrm_policy_afinfo *afinfo)
return -EINVAL;
if (unlikely(afinfo->family >= NPROTO))
return -EAFNOSUPPORT;
- spin_lock_bh(&xfrm_policy_afinfo_lock);
+ spin_lock(&xfrm_policy_afinfo_lock);
if (likely(xfrm_policy_afinfo[afinfo->family] != NULL)) {
if (unlikely(xfrm_policy_afinfo[afinfo->family] != afinfo))
err = -EINVAL;
- else {
- struct dst_ops *dst_ops = afinfo->dst_ops;
- rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
- NULL);
- dst_ops->kmem_cachep = NULL;
- dst_ops->check = NULL;
- dst_ops->negative_advice = NULL;
- dst_ops->link_failure = NULL;
- afinfo->garbage_collect = NULL;
- }
+ else
+ RCU_INIT_POINTER(xfrm_policy_afinfo[afinfo->family],
+ NULL);
+ }
+ spin_unlock(&xfrm_policy_afinfo_lock);
+ if (!err) {
+ struct dst_ops *dst_ops = afinfo->dst_ops;
+
+ synchronize_rcu();
+
+ dst_ops->kmem_cachep = NULL;
+ dst_ops->check = NULL;
+ dst_ops->negative_advice = NULL;
+ dst_ops->link_failure = NULL;
+ afinfo->garbage_collect = NULL;
}
- spin_unlock_bh(&xfrm_policy_afinfo_lock);
- synchronize_rcu();
return err;
}
EXPORT_SYMBOL(xfrm_policy_unregister_afinfo);
@@ -2500,32 +2519,15 @@ static void __net_init xfrm_dst_ops_init(struct net *net)
{
struct xfrm_policy_afinfo *afinfo;
- rcu_read_lock_bh();
- afinfo = rcu_dereference_bh(xfrm_policy_afinfo[AF_INET]);
+ rcu_read_lock();
+ afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET]);
if (afinfo)
net->xfrm.xfrm4_dst_ops = *afinfo->dst_ops;
#if IS_ENABLED(CONFIG_IPV6)
- afinfo = rcu_dereference_bh(xfrm_policy_afinfo[AF_INET6]);
+ afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET6]);
if (afinfo)
net->xfrm.xfrm6_dst_ops = *afinfo->dst_ops;
#endif
- rcu_read_unlock_bh();
-}
-
-static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
-{
- struct xfrm_policy_afinfo *afinfo;
- if (unlikely(family >= NPROTO))
- return NULL;
- rcu_read_lock();
- afinfo = rcu_dereference(xfrm_policy_afinfo[family]);
- if (unlikely(!afinfo))
- rcu_read_unlock();
- return afinfo;
-}
-
-static inline void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo)
-{
rcu_read_unlock();
}
^ permalink raw reply related
* Re: [PATCH 1/5] drivers/net/wireless/ipw2x00/ipw2100.c: introduce missing initialization
From: Stanislav Yakovlev @ 2012-08-19 10:07 UTC (permalink / raw)
To: Julia Lawall
Cc: kernel-janitors-u79uwXL29TY76Z2rM5mHXA, John W. Linville,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <alpine.DEB.2.02.1208191149400.2031-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org>
On 19 August 2012 13:49, Julia Lawall <julia.lawall-L2FTfq7BK8M@public.gmane.org> wrote:
> From: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
>
> The result of one call to a function is tested, and then at the second call
> to the same function, the previous result, and not the current result, is
> tested again.
>
> Also changed &bssid to bssid, at the suggestion of Stanislav Yakovlev.
>
> The semantic match that finds the first problem is as follows:
>
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression ret;
> identifier f;
> statement S1,S2;
> @@
>
> *ret = f(...);
> if (\(ret != 0\|ret < 0\|ret == NULL\)) S1
> ... when any
> *f(...);
> if (\(ret != 0\|ret < 0\|ret == NULL\)) S2
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
>
> ---
> v2: changed &bssid to bssid
>
Thank you, Julia.
Stanislav.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] xfrm:Use rcu_dereference_bh to deference pointer protected by rcu_read_lock_bh
From: Eric Dumazet @ 2012-08-19 9:58 UTC (permalink / raw)
To: David Miller; +Cc: fan.du, fengguang.wu, netdev
In-Reply-To: <20120816.232414.545877018277576779.davem@davemloft.net>
On Thu, 2012-08-16 at 23:24 -0700, David Miller wrote:
> I already applied your patch, as I told you here:
>
> http://marc.info/?l=linux-netdev&m=134517122805719&w=2
>
> This means you are submitting a patch which doesn't not even apply
> to the net-next tree.
>
> Instead of continuing to dig yourself deeper and deeper, take a
> break, take a deep breath, and work slowly and carefully.
> --
BTW, we (incorrectly ?) mix RCU and RCU_BH in this file, and since we
use synchronize_rcu() anyway, we should/could use rcu_read_lock()
everywhere we can, as this is less expensive (currently inlined at least
on x86) than rcu_read_lock_bh()
I'll send a cleanup patch on a separate thread
^ permalink raw reply
* Re: [PATCH 1/5] drivers/net/wireless/ipw2x00/ipw2100.c: introduce missing initialization
From: Julia Lawall @ 2012-08-19 9:49 UTC (permalink / raw)
To: Stanislav Yakovlev
Cc: Julia Lawall, kernel-janitors, John W. Linville, linux-wireless,
netdev, linux-kernel
In-Reply-To: <CA++WF2MmTe6WP6MQRV-v1VwkpcUrw4hU6yh79NdG-K4snzQkbg@mail.gmail.com>
From: Julia Lawall <Julia.Lawall@lip6.fr>
The result of one call to a function is tested, and then at the second call
to the same function, the previous result, and not the current result, is
tested again.
Also changed &bssid to bssid, at the suggestion of Stanislav Yakovlev.
The semantic match that finds the first problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression ret;
identifier f;
statement S1,S2;
@@
*ret = f(...);
if (\(ret != 0\|ret < 0\|ret == NULL\)) S1
... when any
*f(...);
if (\(ret != 0\|ret < 0\|ret == NULL\)) S2
// </smpl>
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
v2: changed &bssid to bssid
drivers/net/wireless/ipw2x00/ipw2100.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ipw2x00/ipw2100.c b/drivers/net/wireless/ipw2x00/ipw2100.c
index 8a34202..534e655 100644
--- a/drivers/net/wireless/ipw2x00/ipw2100.c
+++ b/drivers/net/wireless/ipw2x00/ipw2100.c
@@ -2042,7 +2042,8 @@ static void isr_indicate_associated(struct ipw2100_priv *priv, u32 status)
return;
}
len = ETH_ALEN;
- ipw2100_get_ordinal(priv, IPW_ORD_STAT_ASSN_AP_BSSID, &bssid, &len);
+ ret = ipw2100_get_ordinal(priv, IPW_ORD_STAT_ASSN_AP_BSSID, bssid,
+ &len);
if (ret) {
IPW_DEBUG_INFO("failed querying ordinals at line %d\n",
__LINE__);
^ permalink raw reply related
* Re: [PATCH 1/5] drivers/net/wireless/ipw2x00/ipw2100.c: introduce missing initialization
From: Stanislav Yakovlev @ 2012-08-19 9:36 UTC (permalink / raw)
To: Julia Lawall
Cc: kernel-janitors, John W. Linville, linux-wireless, netdev,
linux-kernel
In-Reply-To: <1345325159-7365-1-git-send-email-Julia.Lawall@lip6.fr>
Hello Julia,
On 19 August 2012 01:25, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> - ipw2100_get_ordinal(priv, IPW_ORD_STAT_ASSN_AP_BSSID, &bssid, &len);
> + ret = ipw2100_get_ordinal(priv, IPW_ORD_STAT_ASSN_AP_BSSID, &bssid,
> + &len);
This is correct, but this line has one more typo. It should be
-&bssid
+bssid
according to the bssid definition in the beginning of the function:
u8 bssid[ETH_ALEN];
Could you fix this as well?
Thanks!
Stanislav.
^ permalink raw reply
* Re: [PATCH] net: tcp: ipv6_mapped needs sk_rx_dst_set method
From: Artem Savkov @ 2012-08-19 8:11 UTC (permalink / raw)
To: Neal Cardwell; +Cc: David Miller, eric.dumazet, akpm, netdev
In-Reply-To: <CADVnQyn-aDMDdb_nfFyZPtmnKkuFtjwfSNWudnhMdYZz7Ddh7g@mail.gmail.com>
On Sun, Aug 19, 2012 at 12:06:33AM -0400, Neal Cardwell wrote:
> On Sat, Aug 18, 2012 at 9:06 AM, Artem Savkov <artem.savkov@gmail.com> wrote:
> > [ 1699.195040] EIP is at inet6_sk_rx_dst_set+0x40/0xa0
> ...
> > [ 1699.198736] [<c15662a1>] tcp_create_openreq_child+0x41/0x4e0
> > [ 1699.198884] [<c1563b54>] tcp_v4_syn_recv_sock+0x34/0x330
> > [ 1699.199032] [<c15d162e>] tcp_v6_syn_recv_sock+0x3fe/0x660
> ...
> > [ 1699.242913] EIP: [<c15d11d0>] inet6_sk_rx_dst_set+0x40/0xa0 SS:ESP 0068:f540dce0
> > [ 1699.245945] CR2: 0000000000000016
> > [ 1699.280708] ---[ end trace 3fb05aeec95e7238 ]---
> > [ 1699.280806] Kernel panic - not syncing: Fatal exception in interrupt
> > [ 1699.284674] panic occurred, switching back to text console
> Thanks for the detailed report!
>
> I don't think that particular fix is kosher, since this basically
> changes the address family ops of the parent listening socket ('sk'
> here).The parent listening socket needs to keep its IPv6 af_ops so its
> IPv6 children can get the right af_ops.
Indeed, my approach breaks ipv6 after first mapped connection.
> We should probably either: (a) make sure the af_ops of the child
> socket are set correctly earlier, or (b) not use dynamic dispatch
> through the af_ops within tcp_create_openreq_child(), and just do the
> sk_rx_dst_set a tiny bit later. I've sent out a patch for approach (b)
> here, since it's simpler:
>
> http://patchwork.ozlabs.org/patch/178525/
>
> I've verified that IPv4, IPv6, and IPv4-mapped-IPv6 connections work
> for me with this patch. But if you could test it as well, that would
> be great.
I've tested it and can confirm that all three connection types work.
Thank you for the patch and explanation.
--
Kind regards,
Artem
^ 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