* [PATCH 03/18] netfilter: nf_conntrack_ipv6: fix tracking of ICMPv6 error messages containing fragments
From: Patrick McHardy @ 2012-08-20 3:39 UTC (permalink / raw)
To: netfilter-devel; +Cc: netdev
In-Reply-To: <1345434006-16549-1-git-send-email-kaber@trash.net>
ICMPv6 error messages are tracked by extracting the conntrack tuple of
the inner packet and looking up the corresponding conntrack entry. Tuple
extraction uses the ->get_l4proto() callback, which in case of fragments
returns NEXTHDR_FRAGMENT instead of the upper protocol, even for the
first fragment when the entire next header is present, resulting in a
failure to find the correct connection tracking entry.
This patch changes ipv6_get_l4proto() to use ipv6_skip_exthdr() instead
of nf_ct_ipv6_skip_exthdr() in order to skip fragment headers when the
fragment offset is zero.
Signed-off-by: Patrick McHardy <kaber@trash.net>
---
net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c | 63 ++---------------------
1 files changed, 6 insertions(+), 57 deletions(-)
diff --git a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
index 521ddca..dcf6010 100644
--- a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
+++ b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
@@ -64,82 +64,31 @@ static int ipv6_print_tuple(struct seq_file *s,
tuple->src.u3.ip6, tuple->dst.u3.ip6);
}
-/*
- * Based on ipv6_skip_exthdr() in net/ipv6/exthdr.c
- *
- * This function parses (probably truncated) exthdr set "hdr"
- * of length "len". "nexthdrp" initially points to some place,
- * where type of the first header can be found.
- *
- * It skips all well-known exthdrs, and returns pointer to the start
- * of unparsable area i.e. the first header with unknown type.
- * if success, *nexthdr is updated by type/protocol of this header.
- *
- * NOTES: - it may return pointer pointing beyond end of packet,
- * if the last recognized header is truncated in the middle.
- * - if packet is truncated, so that all parsed headers are skipped,
- * it returns -1.
- * - if packet is fragmented, return pointer of the fragment header.
- * - ESP is unparsable for now and considered like
- * normal payload protocol.
- * - Note also special handling of AUTH header. Thanks to IPsec wizards.
- */
-
-static int nf_ct_ipv6_skip_exthdr(const struct sk_buff *skb, int start,
- u8 *nexthdrp, int len)
-{
- u8 nexthdr = *nexthdrp;
-
- while (ipv6_ext_hdr(nexthdr)) {
- struct ipv6_opt_hdr hdr;
- int hdrlen;
-
- if (len < (int)sizeof(struct ipv6_opt_hdr))
- return -1;
- if (nexthdr == NEXTHDR_NONE)
- break;
- if (nexthdr == NEXTHDR_FRAGMENT)
- break;
- if (skb_copy_bits(skb, start, &hdr, sizeof(hdr)))
- BUG();
- if (nexthdr == NEXTHDR_AUTH)
- hdrlen = (hdr.hdrlen+2)<<2;
- else
- hdrlen = ipv6_optlen(&hdr);
-
- nexthdr = hdr.nexthdr;
- len -= hdrlen;
- start += hdrlen;
- }
-
- *nexthdrp = nexthdr;
- return start;
-}
-
static int ipv6_get_l4proto(const struct sk_buff *skb, unsigned int nhoff,
unsigned int *dataoff, u_int8_t *protonum)
{
unsigned int extoff = nhoff + sizeof(struct ipv6hdr);
- unsigned char pnum;
+ __be16 frag_off;
int protoff;
+ u8 nexthdr;
if (skb_copy_bits(skb, nhoff + offsetof(struct ipv6hdr, nexthdr),
- &pnum, sizeof(pnum)) != 0) {
+ &nexthdr, sizeof(nexthdr)) != 0) {
pr_debug("ip6_conntrack_core: can't get nexthdr\n");
return -NF_ACCEPT;
}
- protoff = nf_ct_ipv6_skip_exthdr(skb, extoff, &pnum, skb->len - extoff);
+ protoff = ipv6_skip_exthdr(skb, extoff, &nexthdr, &frag_off);
/*
* (protoff == skb->len) mean that the packet doesn't have no data
* except of IPv6 & ext headers. but it's tracked anyway. - YK
*/
- if ((protoff < 0) || (protoff > skb->len)) {
+ if (protoff < 0 || (frag_off & htons(~0x7)) != 0) {
pr_debug("ip6_conntrack_core: can't find proto in pkt\n");
return -NF_ACCEPT;
}
*dataoff = protoff;
- *protonum = pnum;
+ *protonum = nexthdr;
return NF_ACCEPT;
}
--
1.7.1
^ permalink raw reply related
* [PATCH 02/18] netfilter: nf_conntrack_ipv6: improve fragmentation handling
From: Patrick McHardy @ 2012-08-20 3:39 UTC (permalink / raw)
To: netfilter-devel; +Cc: netdev
In-Reply-To: <1345434006-16549-1-git-send-email-kaber@trash.net>
The IPv6 conntrack fragmentation currently has a couple of shortcomings.
Fragmentes are collected in PREROUTING/OUTPUT, are defragmented, the
defragmented packet is then passed to conntrack, the resulting conntrack
information is attached to each original fragment and the fragments then
continue their way through the stack.
Helper invocation occurs in the POSTROUTING hook, at which point only
the original fragments are available. The result of this is that
fragmented packets are never passed to helpers.
This patch improves the situation in the following way:
- If a reassembled packet belongs to a connection that has a helper
assigned, the reassembled packet is passed through the stack instead
of the original fragments.
- During defragmentation, the largest received fragment size is stored.
On output, the packet is refragmented if required. If the largest
received fragment size exceeds the outgoing MTU, a "packet too big"
message is generated, thus behaving as if the original fragments
were passed through the stack from an outside point of view.
- The ipv6_helper() hook function can't receive fragments anymore for
connections using a helper, so it is switched to use ipv6_skip_exthdr()
instead of the netfilter specific nf_ct_ipv6_skip_exthdr() and the
reassembled packets are passed to connection tracking helpers.
The result of this is that we can properly track fragmented packets, but
still generate ICMPv6 Packet too big messages if we would have before.
This patch is also required as a precondition for IPv6 NAT, where NAT
helpers might enlarge packets up to a point that they require
fragmentation. In that case we can't generate Packet too big messages
since the proper MTU can't be calculated in all cases (f.i. when
changing textual representation of a variable amount of addresses),
so the packet is transparently fragmented iff the original packet or
fragments would have fit the outgoing MTU.
Signed-off-by: Patrick McHardy <kaber@trash.net>
---
include/linux/ipv6.h | 1 +
net/ipv6/ip6_output.c | 7 +++-
net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c | 41 ++++++++++++++++++-----
net/ipv6/netfilter/nf_conntrack_reasm.c | 19 +++++++++--
4 files changed, 54 insertions(+), 14 deletions(-)
diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 879db26..0b94e91 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -256,6 +256,7 @@ struct inet6_skb_parm {
#if defined(CONFIG_IPV6_MIP6) || defined(CONFIG_IPV6_MIP6_MODULE)
__u16 dsthao;
#endif
+ __u16 frag_max_size;
#define IP6SKB_XFRM_TRANSFORMED 1
#define IP6SKB_FORWARDED 2
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 5b2d63e..a4f6263 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -493,7 +493,8 @@ int ip6_forward(struct sk_buff *skb)
if (mtu < IPV6_MIN_MTU)
mtu = IPV6_MIN_MTU;
- if (skb->len > mtu && !skb_is_gso(skb)) {
+ if ((!skb->local_df && skb->len > mtu && !skb_is_gso(skb)) ||
+ (IP6CB(skb)->frag_max_size && IP6CB(skb)->frag_max_size > mtu)) {
/* Again, force OUTPUT device used as source address */
skb->dev = dst->dev;
icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
@@ -636,7 +637,9 @@ int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
/* We must not fragment if the socket is set to force MTU discovery
* or if the skb it not generated by a local socket.
*/
- if (unlikely(!skb->local_df && skb->len > mtu)) {
+ if (unlikely(!skb->local_df && skb->len > mtu) ||
+ (IP6CB(skb)->frag_max_size &&
+ IP6CB(skb)->frag_max_size > mtu)) {
if (skb->sk && dst_allfrag(skb_dst(skb)))
sk_nocaps_add(skb->sk, NETIF_F_GSO_MASK);
diff --git a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
index 4794f96..521ddca 100644
--- a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
+++ b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
@@ -153,10 +153,10 @@ static unsigned int ipv6_helper(unsigned int hooknum,
const struct nf_conn_help *help;
const struct nf_conntrack_helper *helper;
enum ip_conntrack_info ctinfo;
- unsigned int ret, protoff;
- unsigned int extoff = (u8 *)(ipv6_hdr(skb) + 1) - skb->data;
- unsigned char pnum = ipv6_hdr(skb)->nexthdr;
-
+ unsigned int ret;
+ __be16 frag_off;
+ int protoff;
+ u8 nexthdr;
/* This is where we call the helper: as the packet goes out. */
ct = nf_ct_get(skb, &ctinfo);
@@ -171,9 +171,10 @@ static unsigned int ipv6_helper(unsigned int hooknum,
if (!helper)
return NF_ACCEPT;
- protoff = nf_ct_ipv6_skip_exthdr(skb, extoff, &pnum,
- skb->len - extoff);
- if (protoff > skb->len || pnum == NEXTHDR_FRAGMENT) {
+ nexthdr = ipv6_hdr(skb)->nexthdr;
+ protoff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &nexthdr,
+ &frag_off);
+ if (protoff < 0 || (frag_off & htons(~0x7)) != 0) {
pr_debug("proto header not found\n");
return NF_ACCEPT;
}
@@ -199,9 +200,14 @@ static unsigned int ipv6_confirm(unsigned int hooknum,
static unsigned int __ipv6_conntrack_in(struct net *net,
unsigned int hooknum,
struct sk_buff *skb,
+ const struct net_device *in,
+ const struct net_device *out,
int (*okfn)(struct sk_buff *))
{
struct sk_buff *reasm = skb->nfct_reasm;
+ const struct nf_conn_help *help;
+ struct nf_conn *ct;
+ enum ip_conntrack_info ctinfo;
/* This packet is fragmented and has reassembled packet. */
if (reasm) {
@@ -213,6 +219,23 @@ static unsigned int __ipv6_conntrack_in(struct net *net,
if (ret != NF_ACCEPT)
return ret;
}
+
+ /* Conntrack helpers need the entire reassembled packet in the
+ * POST_ROUTING hook.
+ */
+ ct = nf_ct_get(reasm, &ctinfo);
+ if (ct != NULL && !nf_ct_is_untracked(ct)) {
+ help = nfct_help(ct);
+ if (help && help->helper) {
+ nf_conntrack_get_reasm(skb);
+ NF_HOOK_THRESH(NFPROTO_IPV6, hooknum, reasm,
+ (struct net_device *)in,
+ (struct net_device *)out,
+ okfn, NF_IP6_PRI_CONNTRACK + 1);
+ return NF_DROP_ERR(-ECANCELED);
+ }
+ }
+
nf_conntrack_get(reasm->nfct);
skb->nfct = reasm->nfct;
skb->nfctinfo = reasm->nfctinfo;
@@ -228,7 +251,7 @@ static unsigned int ipv6_conntrack_in(unsigned int hooknum,
const struct net_device *out,
int (*okfn)(struct sk_buff *))
{
- return __ipv6_conntrack_in(dev_net(in), hooknum, skb, okfn);
+ return __ipv6_conntrack_in(dev_net(in), hooknum, skb, in, out, okfn);
}
static unsigned int ipv6_conntrack_local(unsigned int hooknum,
@@ -242,7 +265,7 @@ static unsigned int ipv6_conntrack_local(unsigned int hooknum,
net_notice_ratelimited("ipv6_conntrack_local: packet too short\n");
return NF_ACCEPT;
}
- return __ipv6_conntrack_in(dev_net(out), hooknum, skb, okfn);
+ return __ipv6_conntrack_in(dev_net(out), hooknum, skb, in, out, okfn);
}
static struct nf_hook_ops ipv6_conntrack_ops[] __read_mostly = {
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index c9c78c2..f94fb3a 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -190,6 +190,7 @@ static int nf_ct_frag6_queue(struct nf_ct_frag6_queue *fq, struct sk_buff *skb,
const struct frag_hdr *fhdr, int nhoff)
{
struct sk_buff *prev, *next;
+ unsigned int payload_len;
int offset, end;
if (fq->q.last_in & INET_FRAG_COMPLETE) {
@@ -197,8 +198,10 @@ static int nf_ct_frag6_queue(struct nf_ct_frag6_queue *fq, struct sk_buff *skb,
goto err;
}
+ payload_len = ntohs(ipv6_hdr(skb)->payload_len);
+
offset = ntohs(fhdr->frag_off) & ~0x7;
- end = offset + (ntohs(ipv6_hdr(skb)->payload_len) -
+ end = offset + (payload_len -
((u8 *)(fhdr + 1) - (u8 *)(ipv6_hdr(skb) + 1)));
if ((unsigned int)end > IPV6_MAXPLEN) {
@@ -307,6 +310,8 @@ found:
skb->dev = NULL;
fq->q.stamp = skb->tstamp;
fq->q.meat += skb->len;
+ if (payload_len > fq->q.max_size)
+ fq->q.max_size = payload_len;
atomic_add(skb->truesize, &nf_init_frags.mem);
/* The first fragment.
@@ -412,10 +417,12 @@ nf_ct_frag6_reasm(struct nf_ct_frag6_queue *fq, struct net_device *dev)
}
atomic_sub(head->truesize, &nf_init_frags.mem);
+ head->local_df = 1;
head->next = NULL;
head->dev = dev;
head->tstamp = fq->q.stamp;
ipv6_hdr(head)->payload_len = htons(payload_len);
+ IP6CB(head)->frag_max_size = sizeof(struct ipv6hdr) + fq->q.max_size;
/* Yes, and fold redundant checksum back. 8) */
if (head->ip_summed == CHECKSUM_COMPLETE)
@@ -592,6 +599,7 @@ void nf_ct_frag6_output(unsigned int hooknum, struct sk_buff *skb,
int (*okfn)(struct sk_buff *))
{
struct sk_buff *s, *s2;
+ unsigned int ret = 0;
for (s = NFCT_FRAG6_CB(skb)->orig; s;) {
nf_conntrack_put_reasm(s->nfct_reasm);
@@ -601,8 +609,13 @@ void nf_ct_frag6_output(unsigned int hooknum, struct sk_buff *skb,
s2 = s->next;
s->next = NULL;
- NF_HOOK_THRESH(NFPROTO_IPV6, hooknum, s, in, out, okfn,
- NF_IP6_PRI_CONNTRACK_DEFRAG + 1);
+ if (ret != -ECANCELED)
+ ret = NF_HOOK_THRESH(NFPROTO_IPV6, hooknum, s,
+ in, out, okfn,
+ NF_IP6_PRI_CONNTRACK_DEFRAG + 1);
+ else
+ kfree_skb(s);
+
s = s2;
}
nf_conntrack_put_reasm(skb);
--
1.7.1
^ permalink raw reply related
* [PATCH 00/18] netfilter: IPv6 NAT
From: Patrick McHardy @ 2012-08-20 3:39 UTC (permalink / raw)
To: netfilter-devel; +Cc: netdev
Following is the latest IPv6 NAT patchset, based on -rc2.
Changes since the last posting include:
- removal of fixes already merged into nf-2.6, not included upstream
yet though, IPv6 SIP will not work
- missing module alias for ip6t_DNAT/ip6t_SNAT added to xt_nat
- compilation fix for ip6t_MASQUERADE without ipt_MASQUERADE enabled
- proper handling of untracked packets in IPv6 fragmentation improvements
- byteorder fixes in IPv6 fragment handling
- conversion to use a mutex for NAT L3/L4 protocol registration
- missing memory barrier before publishing L4 protocol array
- proper cleanup of NATed connections after NAT L3/L4 protocol unregistration
- proper locking (RTNL) for walking network namespaces in NAT cleanup
- only free NAT L4 protocol array at shutdown instead of during L3 protocol
unregistation, otherwise non built-in protocols are missing after L3
protocol reload
- endianess annotation fixes
- missing netlink policy for IPv6 NAT attributes
I'll also push these patches to github, but it is currently acting up,
once it works again they'll be available at
git://github.com/kaber/nf-nat-ipv6.git master
^ permalink raw reply
* [PATCH net-next] ipv4/netfilter: remove unnecessary goto statement for error recovery
From: Patrick McHardy @ 2012-08-20 2:43 UTC (permalink / raw)
To: Jean Sacren; +Cc: netfilter-devel, netdev
> Usually it's a good practice to use goto statement for error recovery
> when initializing the module. This approach could be an overkill if:
>
> 1) there is only one fail case;
> 2) success and failure use the same return statement.
>
> For a cleaner approach, remove the unnecessary goto statement and
> directly implement error recovery.
We have tons of similar cases. And this is much better than directly
adding rollback since it doesn't require changing unrelated code if
anything new is added to the initialization code. Additionally its
usually directly comparable to the cleanup code.
^ permalink raw reply
* [PATCH v2] iproute2: configure: Add search path for 64bit library.
From: Li Wei @ 2012-08-20 1:41 UTC (permalink / raw)
To: shemminger; +Cc: bhutchings, netdev, Li Wei
In-Reply-To: <20120813093323.1506aa83@nehalam.linuxnetplumber.net>
Use pkg-config to tell us the library path and fallback to search
old paths if xtables.pc not exists.
Signed-off-by: Li Wei <lw@cn.fujitsu.com>
---
Changes from v1:
- use pkg-config as Ben and Stephen suggested.
configure | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/configure b/configure
index a1916de..db7cd6a 100755
--- a/configure
+++ b/configure
@@ -148,7 +148,13 @@ check_ipt()
check_ipt_lib_dir()
{
- IPT_LIB_DIR=""
+ IPT_LIB_DIR=$(pkg-config --variable=xtlibdir xtables)
+ if [ -n "$IPT_LIB_DIR" ]; then
+ echo $IPT_LIB_DIR
+ echo "IPT_LIB_DIR:=$IPT_LIB_DIR" >> Config
+ return
+ fi
+
for dir in /lib /usr/lib /usr/local/lib
do
for file in $dir/{xtables,iptables}/lib*t_*so ; do
--
1.7.10.1
^ permalink raw reply related
* Dear Sir/Madam
From: Allen and Violet Large @ 2012-08-20 1:33 UTC (permalink / raw)
--
Dear Sir/Madam,
This is my fifth times of writting you this email since last year till
date but no response from you.Hope you get this one, as this is a
personal
email directed to you. My wife and I won a Jackpot Lotteryof $11.3
million
in July and have voluntarily decided to donate the sum of $500,000.00
USD
to you as part of our own charity project to improve the lot of 10
lucky
individuals all over the world. If you have received this email then
you
are one of the lucky recipients and all you have to do is get back with
us
so that we can send your details to the payout bank.Please note that
you
have to contact my private email for more
informations(allen.violetlarge101@yahoo.com.hk)
You can verify this by visiting the web pages below.
http://gimundo.com/news/article/lottery-winners-allen-and-violet-large-give-11-million-fortune-to-charity/
Goodluck,
Allen and Violet Large
Email:allen.violetlarge101@yahoo.com.hk
^ permalink raw reply
* [PATCH net-next] ipv4/netfilter: remove unnecessary goto statement for error recovery
From: Jean Sacren @ 2012-08-20 1:11 UTC (permalink / raw)
To: netfilter-devel, netdev
Usually it's a good practice to use goto statement for error recovery
when initializing the module. This approach could be an overkill if:
1) there is only one fail case;
2) success and failure use the same return statement.
For a cleaner approach, remove the unnecessary goto statement and
directly implement error recovery.
Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
net/ipv4/netfilter/iptable_filter.c | 6 +-----
net/ipv4/netfilter/iptable_mangle.c | 6 +-----
net/ipv4/netfilter/iptable_raw.c | 6 +-----
3 files changed, 3 insertions(+), 15 deletions(-)
diff --git a/net/ipv4/netfilter/iptable_filter.c b/net/ipv4/netfilter/iptable_filter.c
index 851acec8..9255fd4 100644
--- a/net/ipv4/netfilter/iptable_filter.c
+++ b/net/ipv4/netfilter/iptable_filter.c
@@ -96,14 +96,10 @@ static int __init iptable_filter_init(void)
filter_ops = xt_hook_link(&packet_filter, iptable_filter_hook);
if (IS_ERR(filter_ops)) {
ret = PTR_ERR(filter_ops);
- goto cleanup_table;
+ unregister_pernet_subsys(&iptable_filter_net_ops);
}
return ret;
-
- cleanup_table:
- unregister_pernet_subsys(&iptable_filter_net_ops);
- return ret;
}
static void __exit iptable_filter_fini(void)
diff --git a/net/ipv4/netfilter/iptable_mangle.c b/net/ipv4/netfilter/iptable_mangle.c
index aef5d1f..25549c5 100644
--- a/net/ipv4/netfilter/iptable_mangle.c
+++ b/net/ipv4/netfilter/iptable_mangle.c
@@ -131,14 +131,10 @@ static int __init iptable_mangle_init(void)
mangle_ops = xt_hook_link(&packet_mangler, iptable_mangle_hook);
if (IS_ERR(mangle_ops)) {
ret = PTR_ERR(mangle_ops);
- goto cleanup_table;
+ unregister_pernet_subsys(&iptable_mangle_net_ops);
}
return ret;
-
- cleanup_table:
- unregister_pernet_subsys(&iptable_mangle_net_ops);
- return ret;
}
static void __exit iptable_mangle_fini(void)
diff --git a/net/ipv4/netfilter/iptable_raw.c b/net/ipv4/netfilter/iptable_raw.c
index 07fb710..6acf7ca 100644
--- a/net/ipv4/netfilter/iptable_raw.c
+++ b/net/ipv4/netfilter/iptable_raw.c
@@ -75,14 +75,10 @@ static int __init iptable_raw_init(void)
rawtable_ops = xt_hook_link(&packet_raw, iptable_raw_hook);
if (IS_ERR(rawtable_ops)) {
ret = PTR_ERR(rawtable_ops);
- goto cleanup_table;
+ unregister_pernet_subsys(&iptable_raw_net_ops);
}
return ret;
-
- cleanup_table:
- unregister_pernet_subsys(&iptable_raw_net_ops);
- return ret;
}
static void __exit iptable_raw_fini(void)
^ permalink raw reply related
* Re: [PATCH v1 3/5] cgroup: Protect access to task_cls_classid() when built as module
From: Li Zefan @ 2012-08-20 0:59 UTC (permalink / raw)
To: Neil Horman
Cc: Daniel Wagner, netdev, cgroups, Daniel Wagner, David S. Miller,
Gao feng, Jamal Hadi Salim, John Fastabend, Tejun Heo
In-Reply-To: <20120817182855.GA11607@hmsreliant.think-freely.org>
于 2012/8/18 2:28, Neil Horman 写道:
> On Fri, Aug 17, 2012 at 04:58:12PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>
>> The module version of task_cls_classid() checks if net_cls_sbusys_id
>> is valid to indentify when it is okay to access the controller.
>>
>> Instead relying on the subusys_id to be set, make it explicit
>> with a jump label.
>>
>> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Gao feng <gaofeng@cn.fujitsu.com>
>> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>> Cc: John Fastabend <john.r.fastabend@intel.com>
>> Cc: Li Zefan <lizefan@huawei.com>
>> Cc: Neil Horman <nhorman@tuxdriver.com>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: netdev@vger.kernel.org
>> Cc: cgroups@vger.kernel.org
>> ---
>> include/net/cls_cgroup.h | 5 ++++-
>> net/core/sock.c | 5 +++++
>> net/sched/cls_cgroup.c | 9 +++++++++
>> 3 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
>> index 401672c..bbbd957 100644
>> --- a/include/net/cls_cgroup.h
>> +++ b/include/net/cls_cgroup.h
>> @@ -16,6 +16,7 @@
>> #include <linux/cgroup.h>
>> #include <linux/hardirq.h>
>> #include <linux/rcupdate.h>
>> +#include <linux/jump_label.h>
>>
>> #ifdef CONFIG_CGROUPS
>> struct cgroup_cls_state
>> @@ -44,6 +45,8 @@ static inline u32 task_cls_classid(struct task_struct *p)
>> }
>>
>> #elif IS_MODULE(CONFIG_NET_CLS_CGROUP)
>> +extern struct static_key cgroup_cls_enabled;
>> +#define clscg_enabled static_key_false(&cgroup_cls_enabled)
>>
>> extern int net_cls_subsys_id;
>>
>> @@ -52,7 +55,7 @@ static inline u32 task_cls_classid(struct task_struct *p)
>> int id;
>> u32 classid = 0;
>>
>> - if (in_interrupt())
>> + if (!clscg_enabled || in_interrupt())
>> return 0;
>>
>> rcu_read_lock();
>> diff --git a/net/core/sock.c b/net/core/sock.c
>> index 8f67ced..8106e77 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>> @@ -327,6 +327,11 @@ int __sk_backlog_rcv(struct sock *sk, struct sk_buff *skb)
>> EXPORT_SYMBOL(__sk_backlog_rcv);
>>
>> #if defined(CONFIG_CGROUPS)
>> +#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
>> +struct static_key cgroup_cls_enabled = STATIC_KEY_INIT_FALSE;
>> +EXPORT_SYMBOL_GPL(cgroup_cls_enabled);
>> +#endif
>> +
>> #if !defined(CONFIG_NET_CLS_CGROUP)
>> int net_cls_subsys_id = -1;
>> EXPORT_SYMBOL_GPL(net_cls_subsys_id);
>> diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
>> index 7743ea8..0635894 100644
>> --- a/net/sched/cls_cgroup.c
>> +++ b/net/sched/cls_cgroup.c
>> @@ -44,12 +44,21 @@ static struct cgroup_subsys_state *cgrp_create(struct cgroup *cgrp)
>>
>> if (cgrp->parent)
>> cs->classid = cgrp_cls_state(cgrp->parent)->classid;
>> +#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
>> + else if (!clscg_enabled)
>> + static_key_slow_inc(&cgroup_cls_enabled);
> This is racy I think. The read of the static key is atomic with other reads,
> but the entire conditional is not atomic. If two cpus were creating cgroups in
> parallel, it would be possible for both to read the static key as being zero
> (the second cpu would read the key before the first cpu could increment it).
static_key_slow_inc() is called only when we're creating the root cgroup, and that's
module loading.
so it's safe.
>> +#endif
>>
>> return &cs->css;
>> }
>>
>> static void cgrp_destroy(struct cgroup *cgrp)
>> {
>> +#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
>> + if (!cgrp->parent && clscg_enabled)
>> + static_key_slow_dec(&cgroup_cls_enabled);
> Ditto here with the race above. I think what you want is one of:
and this is called only when we're destroying the root cgroup, and that's
module unload.
^ permalink raw reply
* Re: [PATCH] net/fsl: introduce Freescale 10G MDIO driver
From: Tabi Timur-B04825 @ 2012-08-20 0:59 UTC (permalink / raw)
To: Francois Romieu
Cc: Fleming Andy-AFLEMING, David Miller, netdev@vger.kernel.org
In-Reply-To: <20120819224804.GA28300@electric-eye.fr.zoreil.com>
On Aug 19, 2012, at 6:03 PM, "Francois Romieu" <romieu@fr.zoreil.com> wrote:
> Tabi Timur-B04825 <B04825@freescale.com> :
> [...]
>> I don't see any real savings here.
>
> xgmac_mdio_free_bus_wait could return -ETIMEDOUT and include the
> debug message.
Ok.
>
> --
> Ueimor
>
^ permalink raw reply
* Re: [PATCH] net/fsl: introduce Freescale 10G MDIO driver
From: Francois Romieu @ 2012-08-19 22:48 UTC (permalink / raw)
To: Tabi Timur-B04825
Cc: Fleming Andy-AFLEMING, David Miller, netdev@vger.kernel.org
In-Reply-To: <50315A18.2040105@freescale.com>
Tabi Timur-B04825 <B04825@freescale.com> :
[...]
> I don't see any real savings here.
xgmac_mdio_free_bus_wait could return -ETIMEDOUT and include the
debug message.
--
Ueimor
^ permalink raw reply
* [PATCH v2] serial: add a new helper function
From: Huang Shijie @ 2012-08-19 21:33 UTC (permalink / raw)
To: gregkh; +Cc: alan, jirislaby, linux-kernel, linux-serial, netdev, Huang Shijie
In most of the time, the driver needs to check if the cts flow control
is enabled. But now, the driver checks the ASYNC_CTS_FLOW flag manually,
which is not a grace way. So add a new wraper function to make the code
tidy and clean.
Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
v1 --> v2:
move the new help function to serial.h
---
drivers/char/pcmcia/synclink_cs.c | 2 +-
drivers/tty/amiserial.c | 2 +-
drivers/tty/cyclades.c | 2 +-
drivers/tty/isicom.c | 2 +-
drivers/tty/mxser.c | 2 +-
drivers/tty/serial/mxs-auart.c | 2 +-
drivers/tty/serial/serial_core.c | 4 ++--
drivers/tty/synclink.c | 2 +-
drivers/tty/synclink_gt.c | 2 +-
drivers/tty/synclinkmp.c | 2 +-
include/linux/serial.h | 7 +++++++
net/irda/ircomm/ircomm_tty.c | 4 ++--
net/irda/ircomm/ircomm_tty_attach.c | 2 +-
13 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index 5db08c7..3f57d5de 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -1050,7 +1050,7 @@ static void cts_change(MGSLPC_INFO *info, struct tty_struct *tty)
wake_up_interruptible(&info->status_event_wait_q);
wake_up_interruptible(&info->event_wait_q);
- if (info->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&info->port)) {
if (tty->hw_stopped) {
if (info->serial_signals & SerialSignal_CTS) {
if (debug_level >= DEBUG_LEVEL_ISR)
diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 2b7535d..42d0a25 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -420,7 +420,7 @@ static void check_modem_status(struct serial_state *info)
tty_hangup(port->tty);
}
}
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
if (port->tty->hw_stopped) {
if (!(status & SER_CTS)) {
#if (defined(SERIAL_DEBUG_INTR) || defined(SERIAL_DEBUG_FLOW))
diff --git a/drivers/tty/cyclades.c b/drivers/tty/cyclades.c
index e3954da..0a6a0bc 100644
--- a/drivers/tty/cyclades.c
+++ b/drivers/tty/cyclades.c
@@ -727,7 +727,7 @@ static void cyy_chip_modem(struct cyclades_card *cinfo, int chip,
else
tty_hangup(tty);
}
- if ((mdm_change & CyCTS) && (info->port.flags & ASYNC_CTS_FLOW)) {
+ if ((mdm_change & CyCTS) && tty_port_cts_enabled(&info->port)) {
if (tty->hw_stopped) {
if (mdm_status & CyCTS) {
/* cy_start isn't used
diff --git a/drivers/tty/isicom.c b/drivers/tty/isicom.c
index 99cf22e..d7492e1 100644
--- a/drivers/tty/isicom.c
+++ b/drivers/tty/isicom.c
@@ -600,7 +600,7 @@ static irqreturn_t isicom_interrupt(int irq, void *dev_id)
port->status &= ~ISI_DCD;
}
- if (port->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&port->port)) {
if (tty->hw_stopped) {
if (header & ISI_CTS) {
port->port.tty->hw_stopped = 0;
diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index bb2da4c..cfda47d 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -830,7 +830,7 @@ static void mxser_check_modem_status(struct tty_struct *tty,
wake_up_interruptible(&port->port.open_wait);
}
- if (port->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&port->port)) {
if (tty->hw_stopped) {
if (status & UART_MSR_CTS) {
tty->hw_stopped = 0;
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 3a667ee..dafeef2 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -262,7 +262,7 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
ctrl &= ~AUART_CTRL2_RTSEN;
if (mctrl & TIOCM_RTS) {
- if (u->state->port.flags & ASYNC_CTS_FLOW)
+ if (tty_port_cts_enabled(&u->state->port))
ctrl |= AUART_CTRL2_RTSEN;
}
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 5b308c8..1920e5c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -176,7 +176,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
}
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
spin_lock_irq(&uport->lock);
if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
tty->hw_stopped = 1;
@@ -2493,7 +2493,7 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
uport->icount.cts++;
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
if (tty->hw_stopped) {
if (status) {
tty->hw_stopped = 0;
diff --git a/drivers/tty/synclink.c b/drivers/tty/synclink.c
index 666aa14..70e3a52 100644
--- a/drivers/tty/synclink.c
+++ b/drivers/tty/synclink.c
@@ -1359,7 +1359,7 @@ static void mgsl_isr_io_pin( struct mgsl_struct *info )
}
}
- if ( (info->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&info->port) &&
(status & MISCSTATUS_CTS_LATCHED) ) {
if (info->port.tty->hw_stopped) {
if (status & MISCSTATUS_CTS) {
diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 45f6136..b38e954 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -2053,7 +2053,7 @@ static void cts_change(struct slgt_info *info, unsigned short status)
wake_up_interruptible(&info->event_wait_q);
info->pending_bh |= BH_STATUS;
- if (info->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&info->port)) {
if (info->port.tty) {
if (info->port.tty->hw_stopped) {
if (info->signals & SerialSignal_CTS) {
diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c
index 53429c8..f17d9f3 100644
--- a/drivers/tty/synclinkmp.c
+++ b/drivers/tty/synclinkmp.c
@@ -2500,7 +2500,7 @@ static void isr_io_pin( SLMP_INFO *info, u16 status )
}
}
- if ( (info->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&info->port) &&
(status & MISCSTATUS_CTS_LATCHED) ) {
if ( info->port.tty ) {
if (info->port.tty->hw_stopped) {
diff --git a/include/linux/serial.h b/include/linux/serial.h
index 90e9f98..154dc94 100644
--- a/include/linux/serial.h
+++ b/include/linux/serial.h
@@ -11,6 +11,7 @@
#define _LINUX_SERIAL_H
#include <linux/types.h>
+#include <linux/tty.h>
#ifdef __KERNEL__
#include <asm/page.h>
@@ -218,6 +219,12 @@ struct serial_rs485 {
are a royal PITA .. */
};
+/* 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;
+}
+
#ifdef __KERNEL__
#include <linux/compiler.h>
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 9668990..95a3a7a 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -1070,7 +1070,7 @@ void ircomm_tty_check_modem_status(struct ircomm_tty_cb *self)
goto put;
}
}
- if (tty && self->port.flags & ASYNC_CTS_FLOW) {
+ if (tty && tty_port_cts_enabled(&self->port)) {
if (tty->hw_stopped) {
if (status & IRCOMM_CTS) {
IRDA_DEBUG(2,
@@ -1313,7 +1313,7 @@ static void ircomm_tty_line_info(struct ircomm_tty_cb *self, struct seq_file *m)
seq_puts(m, "Flags:");
sep = ' ';
- if (self->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&self->port)) {
seq_printf(m, "%cASYNC_CTS_FLOW", sep);
sep = '|';
}
diff --git a/net/irda/ircomm/ircomm_tty_attach.c b/net/irda/ircomm/ircomm_tty_attach.c
index 3ab70e7..edab393 100644
--- a/net/irda/ircomm/ircomm_tty_attach.c
+++ b/net/irda/ircomm/ircomm_tty_attach.c
@@ -578,7 +578,7 @@ void ircomm_tty_link_established(struct ircomm_tty_cb *self)
* will have to wait for the peer device (DCE) to raise the CTS
* line.
*/
- if ((self->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&self->port) &&
((self->settings.dce & IRCOMM_CTS) == 0)) {
IRDA_DEBUG(0, "%s(), waiting for CTS ...\n", __func__ );
goto put;
--
1.7.4.4
^ permalink raw reply related
* Re: [PATCH] net/fsl: introduce Freescale 10G MDIO driver
From: Tabi Timur-B04825 @ 2012-08-19 21:26 UTC (permalink / raw)
To: Francois Romieu
Cc: Tabi Timur-B04825, Fleming Andy-AFLEMING, David Miller,
netdev@vger.kernel.org
In-Reply-To: <20120819205724.GA27555@electric-eye.fr.zoreil.com>
Francois Romieu wrote:
>> >+ /* Wait till the bus is free */
>> >+ status = spin_event_timeout(
>> >+ !((in_be32(®s->mdio_stat)) & MDIO_STAT_BSY), TIMEOUT, 0);
>> >+ if (!status) {
>> >+ dev_dbg(&bus->dev, "%s: timeout waiting for stat\n", __func__);
>> >+ return -ETIMEDOUT;
>> >+ }
> This code - comment included - is repeated several times. You may
> consider factoring it out in some xgmac_mdio_free_bus_wait function.
But it's just one line and a test. The function would look like this:
bool xgmac_mdio_free_bus_wait(struct tgec_mdio_controller __iomem *regs)
{
uint32_t status;
status = spin_event_timeout(
!((in_be32(®s->mdio_stat)) & MDIO_STAT_BSY), TIMEOUT, 0);
return status != 0;
}
and then
if (!xgmac_mdio_free_bus_wait(regs)) {
dev_dbg(&bus->dev, "%s: timeout waiting for stat\n", __func__);
return -ETIMEDOUT;
}
I don't see any real savings here.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH 2/2] [RFC] netlink: fix possible spoofing from non-root processes
From: Pablo Neira Ayuso @ 2012-08-19 21:23 UTC (permalink / raw)
To: netdev; +Cc: davem
In-Reply-To: <1345224149-5946-3-git-send-email-pablo@netfilter.org>
[-- Attachment #1: Type: text/plain, Size: 550 bytes --]
On Fri, Aug 17, 2012 at 07:22:29PM +0200, pablo@netfilter.org wrote:
[...]
> [ I don't know any FOSS program making use of Netlink to communicate
> to processes, please, let me know if I'm missing anyone important ]
Patrick pinged me for little reminder on NETLINK_USERSOCK. We still
have to allow netlink-to-netlink userspace communication for it.
So, please find a new version of this patch that allows non-root
processes for that Netlink bus. For others, my patch restricts to root
processes the ability of sending messages with dst_pid != 0.
[-- Attachment #2: 0001-RFC-netlink-fix-possible-spoofing-from-non-root-proc.patch --]
[-- Type: text/x-diff, Size: 3095 bytes --]
>From 78ed359b9802569caebbf2d3507d08d6c7204a84 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 16 Aug 2012 17:58:18 +0200
Subject: [PATCH] [RFC] netlink: fix possible spoofing from non-root processes
Non-root user-space processes can send netlink messages to other
processes that are well-known for being subscribed to Netlink
asynchronous notifications. This allows ilegitimate non-root
process to send forged messages to them.
This is usually fixed by checking for Netlink portID in the
message receival path of the user-space process. In general,
portID == 0 means that the origin of the messages comes from the
kernel. Thus, discarding any message not coming from the kernel.
This is true for rtnetlink.
However, ctnetlink sets the portID in event messages that has
been triggered by some user-space process, eg. conntrack utility.
So other processes subscribed to ctnetlink events, eg. conntrackd,
know that the event was triggered by some user-space action.
This patch adds capability validation in case that dst_pid is set
in netlink_sendmsg(). This approach is aggressive since any existing
application using any of the Netlink busses to deliver messages
between two user-space processes will break.
[ Patrick McHardy has pinged me to let me know about NETLINK_USERSOCK.
Thus, this patch sets NL_CFG_F_NONROOT_SEND to allow non-root
netlink-to-netlink userspace communication for that Netlink bus. ]
However, if we want to ensure full backward compatibility, a new
version of this patch including NL_CFG_F_NONROOT_SEND flags need
to be set in all kernel subsystems. However, I don't think it
makes sense to use NETLINK_ROUTE to communicate two processes
that are sending no matter what information that is not related
to link/neighbouring/routing?
Still, if someone wants to make use of Netlink for this, eg.
I remember people willing to implement D-BUS over Netlink,
then they can reserve some Netlink bus explicitly for this and
set NL_CFG_F_NONROOT_SEND to it.
Reported-by: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netlink/af_netlink.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index d04f923..b3e0e2c 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1373,7 +1373,8 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock,
dst_pid = addr->nl_pid;
dst_group = ffs(addr->nl_groups);
err = -EPERM;
- if (dst_group && !netlink_capable(sock, NL_CFG_F_NONROOT_SEND))
+ if ((dst_group || dst_pid) &&
+ !netlink_capable(sock, NL_CFG_F_NONROOT_SEND))
goto out;
} else {
dst_pid = nlk->dst_pid;
@@ -2141,6 +2142,7 @@ static void __init netlink_add_usersock_entry(void)
rcu_assign_pointer(nl_table[NETLINK_USERSOCK].listeners, listeners);
nl_table[NETLINK_USERSOCK].module = THIS_MODULE;
nl_table[NETLINK_USERSOCK].registered = 1;
+ nl_table[NETLINK_USERSOCK].flags = NL_CFG_F_NONROOT_SEND;
netlink_table_ungrab();
}
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH] net/fsl: introduce Freescale 10G MDIO driver
From: Francois Romieu @ 2012-08-19 20:57 UTC (permalink / raw)
To: Timur Tabi; +Cc: Andy Fleming, David Miller, netdev
In-Reply-To: <1345233792-14985-1-git-send-email-timur@freescale.com>
Timur Tabi <timur@freescale.com> :
> Similar to fsl_pq_mdio.c, this driver is for the 10G MDIO controller on
> Freescale Frame Manager Ethernet controllers.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
[...]
> diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
> new file mode 100644
> index 0000000..1894476
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
[...]
> +static int xgmac_mdio_write(struct mii_bus *bus, int phy_id, int regnum, u16 value)
> +{
> + struct tgec_mdio_controller __iomem *regs = bus->priv;
> + uint16_t dev_addr = regnum >> 16;
> + uint32_t status;
> +
> + /* Setup the MII Mgmt clock speed */
> + out_be32(®s->mdio_stat, MDIO_STAT_CLKDIV(100));
> +
> + /* Wait till the bus is free */
> + status = spin_event_timeout(
> + !((in_be32(®s->mdio_stat)) & MDIO_STAT_BSY), TIMEOUT, 0);
> + if (!status) {
> + dev_dbg(&bus->dev, "%s: timeout waiting for stat\n", __func__);
> + return -ETIMEDOUT;
> + }
This code - comment included - is repeated several times. You may
consider factoring it out in some xgmac_mdio_free_bus_wait function.
--
Ueimor
^ permalink raw reply
* Re: network packet corruption in v3.6.0-rc1 (and also in v3.5)
From: Andrew Worsley @ 2012-08-19 20:20 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Alexey Kuznetsov, James Morris, netdev
In-Reply-To: <1345374134.5158.103.camel@edumazet-glaptop>
> 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 ?
Unfortunately I don't have another USB dongle. I could try to get the built in
Broadcom Corporation BCM4331 802.11a/b/g/n (rev 02)
Subsystem: Apple Inc. Device 00ef
but I need to dig out the proprietary firmware - which will take some
time as I am not familiar with the procedure.
The weird thing about this USB dongle is that I brought it because it
was claimed to be supported under Linux
idVendor 0x0b05 ASUSTek Computer, Inc.
idProduct 0x1786 USB-N10 802.11n Network Adapter [Realtek RTL8188SU]
RealTek driver web site - -
http://www.realtek.com.tw/downloads/downloadsView.aspx?Langid=1&PNid=48&PFid=48&Level=5&Conn=4&DownTypeID=3&GetDown=false&Downloads=true#RTL8188SU
which claims support "Linux Kernel 2.6.18~2.6.38 and Kernel 3.0.2"
but requires I try compiling the driver from source
Andrew
^ permalink raw reply
* Re: [PATCH 05/19] netfilter: nf_conntrack_ipv6: improve fragmentation handling
From: Patrick McHardy @ 2012-08-19 19:44 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: netfilter-devel, netdev
In-Reply-To: <1345405069.3069.241.camel@localhost>
On Sun, 19 Aug 2012, Jesper Dangaard Brouer wrote:
> On Sat, 2012-08-18 at 14:26 +0200, Patrick McHardy wrote:
>>>
>>> Could you provide an iptables command/rule, that trigger this code path?
>>
>> The easiest way is a large ping with the NAT patches also applied,
>> in that case we also pass the first packet of a connection through
>> the stack reassembled.
>
> So, a fragmented IPv6 ICMPv6 packet, I assume?
Correct.
> Don't I need to load some of the helper modules, or just the
> nf_conntrack_ipv6 module, or perhaps only nf_defrag_ipv6 ?
Not with the entire patchset, just IPv6 conntrack is enough. Aith IPv6 NAT
the first packet of a connection must always be defragemented, independant
of an assigned helper.
>>>> @@ -199,9 +200,13 @@ static unsigned int ipv6_confirm(unsigned int hooknum,
>>>> static unsigned int __ipv6_conntrack_in(struct net *net,
>>>> unsigned int hooknum,
>>>> struct sk_buff *skb,
>>>> + const struct net_device *in,
>>>> + const struct net_device *out,
>>>> int (*okfn)(struct sk_buff *))
>>>> {
>>>> struct sk_buff *reasm = skb->nfct_reasm;
>>>> + struct nf_conn *ct;
>>>> + enum ip_conntrack_info ctinfo;
>>>>
>>>> /* This packet is fragmented and has reassembled packet. */
>>>> if (reasm) {
>>>> @@ -213,6 +218,20 @@ static unsigned int __ipv6_conntrack_in(struct net *net,
>>>> if (ret != NF_ACCEPT)
>>>> return ret;
>>>> }
>>>> +
>>>> + /* Conntrack helpers need the entire reassembled packet in the
>>>> + * POST_ROUTING hook.
>>>> + */
>>>> + ct = nf_ct_get(reasm, &ctinfo);
>>>> + if (ct != NULL && test_bit(IPS_HELPER_BIT, &ct->status)) {
>>>> + nf_conntrack_get_reasm(skb);
>>>> + NF_HOOK_THRESH(NFPROTO_IPV6, hooknum, reasm,
>>>> + (struct net_device *)in,
>>>> + (struct net_device *)out,
>>>> + okfn, NF_IP6_PRI_CONNTRACK + 1);
>>>
>>> Hook prio change to NF_IP6_PRI_CONNTRACK + 1
>>
>> I didn't get this part, you want to change to PRE_CONNTRACK + 1? What
>> about raw and SELinux?
>
> No - I don't want any changes.
>
> I was just pointing out *where* the changes occur in your patch. This is
> just a "service" to other email readers, so they can spot the changes
> faster, I were referring to.
Could you send me your patch so I get a better picture of what you're
doing exactly?
^ permalink raw reply
* Re: [PATCH 05/19] netfilter: nf_conntrack_ipv6: improve fragmentation handling
From: Jesper Dangaard Brouer @ 2012-08-19 19:37 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netfilter-devel, netdev
In-Reply-To: <Pine.GSO.4.63.1208181423050.17600@stinky-local.trash.net>
On Sat, 2012-08-18 at 14:26 +0200, Patrick McHardy wrote:
> On Fri, 17 Aug 2012, Jesper Dangaard Brouer wrote:
>
> > On Thu, 2012-08-09 at 22:08 +0200, kaber@trash.net wrote:
> >> From: Patrick McHardy <kaber@trash.net>
> >>
> >> The IPv6 conntrack fragmentation currently has a couple of shortcomings.
> >> Fragmentes are collected in PREROUTING/OUTPUT, are defragmented, the
> >> defragmented packet is then passed to conntrack, the resulting conntrack
> >> information is attached to each original fragment and the fragments then
> >> continue their way through the stack.
> >>
> >> Helper invocation occurs in the POSTROUTING hook, at which point only
> >> the original fragments are available. The result of this is that
> >> fragmented packets are never passed to helpers.
> >>
> >> This patch improves the situation in the following way:
> >>
> >> - If a reassembled packet belongs to a connection that has a helper
> >> assigned, the reassembled packet is passed through the stack instead
> >> of the original fragments.
> >
> > I'm working on IPv6 fragment handling for IPVS, and are taking advantage
> > of the "replay" by nf_ct_frag6_output() at hook prio -399
> > (NF_IP6_PRI_CONNTRACK_DEFRAG + 1).
> > By making a hook at NF_INET_PRE_ROUTING at prio -99 (NF_IP6_PRI_NAT_DST
> > + 1).
> >
> > I can see that the code path can be changed (with this patch), if a
> > helper is assigned. Then the "replay" starts at prio -199
> > (NF_IP6_PRI_CONNTRACK + 1), I guess I'm safe as I run at -99.
> >
> > I have tested that your patchset works, with my ipvs patches, but would
> > like the trigger the changed code path, to make sure.
> >
> > Could you provide an iptables command/rule, that trigger this code path?
>
> The easiest way is a large ping with the NAT patches also applied,
> in that case we also pass the first packet of a connection through
> the stack reassembled.
So, a fragmented IPv6 ICMPv6 packet, I assume?
Don't I need to load some of the helper modules, or just the
nf_conntrack_ipv6 module, or perhaps only nf_defrag_ipv6 ?
> >> @@ -199,9 +200,13 @@ static unsigned int ipv6_confirm(unsigned int hooknum,
> >> static unsigned int __ipv6_conntrack_in(struct net *net,
> >> unsigned int hooknum,
> >> struct sk_buff *skb,
> >> + const struct net_device *in,
> >> + const struct net_device *out,
> >> int (*okfn)(struct sk_buff *))
> >> {
> >> struct sk_buff *reasm = skb->nfct_reasm;
> >> + struct nf_conn *ct;
> >> + enum ip_conntrack_info ctinfo;
> >>
> >> /* This packet is fragmented and has reassembled packet. */
> >> if (reasm) {
> >> @@ -213,6 +218,20 @@ static unsigned int __ipv6_conntrack_in(struct net *net,
> >> if (ret != NF_ACCEPT)
> >> return ret;
> >> }
> >> +
> >> + /* Conntrack helpers need the entire reassembled packet in the
> >> + * POST_ROUTING hook.
> >> + */
> >> + ct = nf_ct_get(reasm, &ctinfo);
> >> + if (ct != NULL && test_bit(IPS_HELPER_BIT, &ct->status)) {
> >> + nf_conntrack_get_reasm(skb);
> >> + NF_HOOK_THRESH(NFPROTO_IPV6, hooknum, reasm,
> >> + (struct net_device *)in,
> >> + (struct net_device *)out,
> >> + okfn, NF_IP6_PRI_CONNTRACK + 1);
> >
> > Hook prio change to NF_IP6_PRI_CONNTRACK + 1
>
> I didn't get this part, you want to change to PRE_CONNTRACK + 1? What
> about raw and SELinux?
No - I don't want any changes.
I was just pointing out *where* the changes occur in your patch. This is
just a "service" to other email readers, so they can spot the changes
faster, I were referring to.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: regression with poll(2)
From: Sage Weil @ 2012-08-19 18:49 UTC (permalink / raw)
To: mgorman, davem, netdev
Cc: linux-kernel, ceph-devel, neilb, a.p.zijlstra, michaelc, emunson,
eric.dumazet, sebastian, cl, akpm, torvalds
In-Reply-To: <alpine.DEB.2.00.1208151226250.15721@cobra.newdream.net>
I've bisected and identified this commit:
netvm: propagate page->pfmemalloc to skb
The skb->pfmemalloc flag gets set to true iff during the slab allocation
of data in __alloc_skb that the the PFMEMALLOC reserves were used. If the
packet is fragmented, it is possible that pages will be allocated from the
PFMEMALLOC reserve without propagating this information to the skb. This
patch propagates page->pfmemalloc from pages allocated for fragments to
the skb.
Signed-off-by: Mel Gorman <mgorman@suse.de>
Acked-by: David S. Miller <davem@davemloft.net>
Cc: Neil Brown <neilb@suse.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Eric B Munson <emunson@mgebm.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
I've retested several times and confirmed that this change leads to the
breakage, and also confirmed that reverting it on top of -rc1 also fixes
the problem.
I've also added some additional instrumentation to my code and confirmed
that the process is blocking on poll(2) while netstat is reporting
data available on the socket.
What can I do to help track this down?
Thanks!
sage
On Wed, 15 Aug 2012, Sage Weil wrote:
> I'm experiencing a stall with Ceph daemons communicating over TCP that
> occurs reliably with 3.6-rc1 (and linus/master) but not 3.5. The basic
> situation is:
>
> - the socket is two processes communicating over TCP on the same host, e.g.
>
> tcp 0 2164849 10.214.132.38:6801 10.214.132.38:51729 ESTABLISHED
>
> - one end writes a bunch of data in
> - the other end consumes data, but at some point stalls.
> - reads are nonblocking, e.g.
>
> int got = ::recv( sd, buf, len, MSG_DONTWAIT );
>
> and between those calls we wait with
>
> struct pollfd pfd;
> short evmask;
> pfd.fd = sd;
> pfd.events = POLLIN;
> #if defined(__linux__)
> pfd.events |= POLLRDHUP;
> #endif
>
> if (poll(&pfd, 1, msgr->timeout) <= 0)
> return -1;
>
> - in my case the timeout is ~15 minutes. at that point it errors out,
> and the daemons reconnect and continue for a while until hitting this
> again.
>
> - at the time of the stall, the reading process is blocked on that
> poll(2) call. There are a bunch of threads stuck on poll(2), some of them
> stuck and some not, but they all have stacks like
>
> [<ffffffff8118f6f9>] poll_schedule_timeout+0x49/0x70
> [<ffffffff81190baf>] do_sys_poll+0x35f/0x4c0
> [<ffffffff81190deb>] sys_poll+0x6b/0x100
> [<ffffffff8163d369>] system_call_fastpath+0x16/0x1b
>
> - you'll note that the netstat output shows data queued:
>
> tcp 0 1163264 10.214.132.36:6807 10.214.132.36:41738 ESTABLISHED
> tcp 0 1622016 10.214.132.36:41738 10.214.132.36:6807 ESTABLISHED
>
> etc.
>
> Is this a known regression? Or might I be misusing the API? What
> information would help track it down?
>
> Thanks!
> sage
>
>
>
^ permalink raw reply
* [PATCH] serial: add a new helper function
From: Huang Shijie @ 2012-08-19 18:27 UTC (permalink / raw)
To: gregkh; +Cc: alan, jirislaby, linux-kernel, linux-serial, netdev, Huang Shijie
In most of the time, the driver needs to check if the cts flow control
is enabled. But now, the driver checks the ASYNC_CTS_FLOW flag manually,
which is not a grace way. So add a new wraper function to make the code
tidy and clean.
Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
drivers/char/pcmcia/synclink_cs.c | 2 +-
drivers/tty/amiserial.c | 2 +-
drivers/tty/cyclades.c | 2 +-
drivers/tty/isicom.c | 2 +-
drivers/tty/mxser.c | 2 +-
drivers/tty/serial/mxs-auart.c | 2 +-
drivers/tty/serial/serial_core.c | 4 ++--
drivers/tty/synclink.c | 2 +-
drivers/tty/synclink_gt.c | 2 +-
drivers/tty/synclinkmp.c | 2 +-
include/linux/tty.h | 7 +++++++
net/irda/ircomm/ircomm_tty.c | 4 ++--
net/irda/ircomm/ircomm_tty_attach.c | 2 +-
13 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index 5db08c7..3f57d5de 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -1050,7 +1050,7 @@ static void cts_change(MGSLPC_INFO *info, struct tty_struct *tty)
wake_up_interruptible(&info->status_event_wait_q);
wake_up_interruptible(&info->event_wait_q);
- if (info->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&info->port)) {
if (tty->hw_stopped) {
if (info->serial_signals & SerialSignal_CTS) {
if (debug_level >= DEBUG_LEVEL_ISR)
diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 2b7535d..42d0a25 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -420,7 +420,7 @@ static void check_modem_status(struct serial_state *info)
tty_hangup(port->tty);
}
}
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
if (port->tty->hw_stopped) {
if (!(status & SER_CTS)) {
#if (defined(SERIAL_DEBUG_INTR) || defined(SERIAL_DEBUG_FLOW))
diff --git a/drivers/tty/cyclades.c b/drivers/tty/cyclades.c
index e3954da..0a6a0bc 100644
--- a/drivers/tty/cyclades.c
+++ b/drivers/tty/cyclades.c
@@ -727,7 +727,7 @@ static void cyy_chip_modem(struct cyclades_card *cinfo, int chip,
else
tty_hangup(tty);
}
- if ((mdm_change & CyCTS) && (info->port.flags & ASYNC_CTS_FLOW)) {
+ if ((mdm_change & CyCTS) && tty_port_cts_enabled(&info->port)) {
if (tty->hw_stopped) {
if (mdm_status & CyCTS) {
/* cy_start isn't used
diff --git a/drivers/tty/isicom.c b/drivers/tty/isicom.c
index 99cf22e..d7492e1 100644
--- a/drivers/tty/isicom.c
+++ b/drivers/tty/isicom.c
@@ -600,7 +600,7 @@ static irqreturn_t isicom_interrupt(int irq, void *dev_id)
port->status &= ~ISI_DCD;
}
- if (port->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&port->port)) {
if (tty->hw_stopped) {
if (header & ISI_CTS) {
port->port.tty->hw_stopped = 0;
diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index bb2da4c..cfda47d 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -830,7 +830,7 @@ static void mxser_check_modem_status(struct tty_struct *tty,
wake_up_interruptible(&port->port.open_wait);
}
- if (port->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&port->port)) {
if (tty->hw_stopped) {
if (status & UART_MSR_CTS) {
tty->hw_stopped = 0;
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 3a667ee..dafeef2 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -262,7 +262,7 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
ctrl &= ~AUART_CTRL2_RTSEN;
if (mctrl & TIOCM_RTS) {
- if (u->state->port.flags & ASYNC_CTS_FLOW)
+ if (tty_port_cts_enabled(&u->state->port))
ctrl |= AUART_CTRL2_RTSEN;
}
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 5b308c8..1920e5c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -176,7 +176,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
}
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
spin_lock_irq(&uport->lock);
if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
tty->hw_stopped = 1;
@@ -2493,7 +2493,7 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
uport->icount.cts++;
- if (port->flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(port)) {
if (tty->hw_stopped) {
if (status) {
tty->hw_stopped = 0;
diff --git a/drivers/tty/synclink.c b/drivers/tty/synclink.c
index 666aa14..70e3a52 100644
--- a/drivers/tty/synclink.c
+++ b/drivers/tty/synclink.c
@@ -1359,7 +1359,7 @@ static void mgsl_isr_io_pin( struct mgsl_struct *info )
}
}
- if ( (info->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&info->port) &&
(status & MISCSTATUS_CTS_LATCHED) ) {
if (info->port.tty->hw_stopped) {
if (status & MISCSTATUS_CTS) {
diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 45f6136..b38e954 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -2053,7 +2053,7 @@ static void cts_change(struct slgt_info *info, unsigned short status)
wake_up_interruptible(&info->event_wait_q);
info->pending_bh |= BH_STATUS;
- if (info->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&info->port)) {
if (info->port.tty) {
if (info->port.tty->hw_stopped) {
if (info->signals & SerialSignal_CTS) {
diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c
index 53429c8..f17d9f3 100644
--- a/drivers/tty/synclinkmp.c
+++ b/drivers/tty/synclinkmp.c
@@ -2500,7 +2500,7 @@ static void isr_io_pin( SLMP_INFO *info, u16 status )
}
}
- if ( (info->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&info->port) &&
(status & MISCSTATUS_CTS_LATCHED) ) {
if ( info->port.tty ) {
if (info->port.tty->hw_stopped) {
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 69a787f..e0b4871 100644
--- 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;
+}
+
extern struct tty_struct *tty_port_tty_get(struct tty_port *port);
extern void tty_port_tty_set(struct tty_port *port, struct tty_struct *tty);
extern int tty_port_carrier_raised(struct tty_port *port);
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 9668990..95a3a7a 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -1070,7 +1070,7 @@ void ircomm_tty_check_modem_status(struct ircomm_tty_cb *self)
goto put;
}
}
- if (tty && self->port.flags & ASYNC_CTS_FLOW) {
+ if (tty && tty_port_cts_enabled(&self->port)) {
if (tty->hw_stopped) {
if (status & IRCOMM_CTS) {
IRDA_DEBUG(2,
@@ -1313,7 +1313,7 @@ static void ircomm_tty_line_info(struct ircomm_tty_cb *self, struct seq_file *m)
seq_puts(m, "Flags:");
sep = ' ';
- if (self->port.flags & ASYNC_CTS_FLOW) {
+ if (tty_port_cts_enabled(&self->port)) {
seq_printf(m, "%cASYNC_CTS_FLOW", sep);
sep = '|';
}
diff --git a/net/irda/ircomm/ircomm_tty_attach.c b/net/irda/ircomm/ircomm_tty_attach.c
index 3ab70e7..edab393 100644
--- a/net/irda/ircomm/ircomm_tty_attach.c
+++ b/net/irda/ircomm/ircomm_tty_attach.c
@@ -578,7 +578,7 @@ void ircomm_tty_link_established(struct ircomm_tty_cb *self)
* will have to wait for the peer device (DCE) to raise the CTS
* line.
*/
- if ((self->port.flags & ASYNC_CTS_FLOW) &&
+ if (tty_port_cts_enabled(&self->port) &&
((self->settings.dce & IRCOMM_CTS) == 0)) {
IRDA_DEBUG(0, "%s(), waiting for CTS ...\n", __func__ );
goto put;
--
1.7.4.4
^ permalink raw reply related
* Re: BUG: unable to handle kernel paging request at 00010016
From: Shaun Ruffell @ 2012-08-19 17:19 UTC (permalink / raw)
To: Artem Savkov; +Cc: Dave Haywood, Linux Kernel, netdev
In-Reply-To: <20120819082117.GA4634@thinkpad.lan>
[ Fixing netdev cc to use proper dadress... ]
On Sun, Aug 19, 2012 at 12:21:17PM +0400, Artem Savkov wrote:
> On Sat, Aug 18, 2012 at 11:25:43PM -0500, Shaun Ruffell wrote:
> > Adding linux-net to the CC list.
> >
> > On Fri, Aug 17, 2012 at 11:57:56PM +0100, Dave Haywood wrote:
> > > [1.] One line summary of the problem:
> > > BUG: unable to handle kernel paging request at 00010016
> > >
> > > System boots then crashes a 5-10 or so seconds after getting to the login prompt
> > > Booting without the network cable attached prevents the crash (no evidence beyond 10 minutes after boot)
> > >
> > > Diagnostics:
> > > Captured the boot and managed a login + dmesg before the crash
> > > Some of the log looks corrupted. Probably my crappy usb dongle serial flow control but left it in anyway
> >
> > [snip]
> >
> > Just a note that I see this as well. It happens reliably for me after trying to
> > login to the machine via ssh.
> >
> > Here is the back trace I collected on the serial port.
>
> There is a patch posted on netdev that fixes this for me:
> http://patchwork.ozlabs.org/patch/178525/
Thanks, I just applied that patch and confirmed that it does indeed
resolve the crash for me as well.
Cheers,
Shaun
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Julian Anastasov @ 2012-08-19 16:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <1345380682.5158.201.camel@edumazet-glaptop>
Hello,
On Sun, 19 Aug 2012, Eric Dumazet wrote:
> 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...
Above code is fine as long as caller holds reference.
Your change for tcp_release_cb looks correct.
Also, may be tcp_v4_mtu_reduced is missing a check
for TCP_CLOSE state? tcp_v6_mtu_reduced already has such
check.
> void sk_reset_timer(struct sock *sk, struct timer_list* timer,
> unsigned long expires)
> {
This should not be needed:
> sock_hold(sk);
> if (mod_timer(timer, expires))
> sock_put(sk);
> }
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH v2 01/16] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-19 16:17 UTC (permalink / raw)
To: Mathieu Desnoyers
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: <20120819141641.GA9082@Krystal>
On 08/19/2012 04:16 PM, Mathieu Desnoyers wrote:
> * 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().
I'd rather avoid starting to use chain/bucket since they're not used anywhere
else (I've tried keeping internal hashing/bucketing opaque to the user).
Otherwise makes sense, I'll improve the documentation as suggested. Thanks!
> Thanks,
>
> Mathieu
>
^ permalink raw reply
* Re: [PATCH v2 01/16] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-19 16:08 UTC (permalink / raw)
To: Mathieu Desnoyers
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>
On 08/19/2012 03:16 PM, Mathieu Desnoyers wrote:
> * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> 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 n I'd ot 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, whiy och is more
> reasonable.
This is actually how the hashtable API was looking in the first place - without
the _size functions.
The story here is that when I've introduced the original API which used macro
magic to work out the size, one of the first comments was that there are several
dynamically allocated hashtables around the kernels, and all this macro magic
wouldn't work.
I've grepped around the code and indeed saw several places which k*alloc/vmalloc
their hashtable, so I agreed with that and happily went ahead to extend the API
to have _size functions.
When I started converting more kernel code to use this new API, I also converted
two modules which kmalloced the hashtable, but instead of using the _size API I
ended up removing the allocation completely because it was unnecessary and
wasteful. And this is why you don't see _size being used anywhere in any of the
patches.
Looking at the kernel code again, I see several places where removal of dynamic
allocation won't work (see 'new_tl_hash' in drivers/block/drbd/drbd_nl.c for
example).
So I'd rather leave it in at least until we finish converting, as I see several
places which will definitely need it.
^ permalink raw reply
* Re: IPv4 BUG: held lock freed!
From: Eric Dumazet @ 2012-08-19 15:55 UTC (permalink / raw)
To: Lin Ming; +Cc: Fengguang Wu, David Miller, networking, LKML
In-Reply-To: <CAF1ivSZK8Y2XpjUXVpNg=m=bs7bGCYqF0ZQU2u_YpEUrmOrR-g@mail.gmail.com>
On Sun, 2012-08-19 at 23:05 +0800, Lin Ming wrote:
> 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?
>
No problem.
It always been like that. Thats the whole point having a refcount at the
first place.
The last sock_put(sk) should free the socket.
^ permalink raw reply
* 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
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