* Re: [RFC PATCH v3 4/5] skb: add tracepoints to freeing skb
From: Koki Sanagi @ 2010-07-21 7:02 UTC (permalink / raw)
To: Neil Horman
Cc: netdev, linux-kernel, davem, kaneshige.kenji, izumi.taku,
kosaki.motohiro, laijs, scott.a.mcmillan, rostedt, eric.dumazet,
fweisbec, mathieu.desnoyers
In-Reply-To: <20100720115044.GD1995@hmsreliant.think-freely.org>
(2010/07/20 20:50), Neil Horman wrote:
> On Tue, Jul 20, 2010 at 09:49:10AM +0900, Koki Sanagi wrote:
>> [RFC PATCH v3 4/5] skb: add tracepoints to freeing skb
>> This patch adds tracepoint to consume_skb, dev_kfree_skb_irq and
>> skb_free_datagram_locked. Combinating with tracepoint on dev_hard_start_xmit,
>> we can check how long it takes to free transmited packets. And using it, we can
>> calculate how many packets driver had at that time. It is useful when a drop of
>> transmited packet is a problem.
>>
>> <idle>-0 [001] 241409.218333: consume_skb: skbaddr=dd6b2fb8
>> <idle>-0 [001] 241409.490555: dev_kfree_skb_irq: skbaddr=f5e29840
>>
>> udp-recv-302 [001] 515031.206008: skb_free_datagram_locked: skbaddr=f5b1d900
>>
>>
>> Signed-off-by: Koki Sanagi <sanagi.koki@jp.fujitsu.com>
>> ---
>> include/trace/events/skb.h | 42 ++++++++++++++++++++++++++++++++++++++++++
>> net/core/datagram.c | 1 +
>> net/core/dev.c | 2 ++
>> net/core/skbuff.c | 1 +
>> 4 files changed, 46 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
>> index 4b2be6d..84c9041 100644
>> --- a/include/trace/events/skb.h
>> +++ b/include/trace/events/skb.h
>> @@ -35,6 +35,48 @@ TRACE_EVENT(kfree_skb,
>> __entry->skbaddr, __entry->protocol, __entry->location)
>> );
>>
>> +DECLARE_EVENT_CLASS(free_skb,
>> +
>> + TP_PROTO(struct sk_buff *skb),
>> +
>> + TP_ARGS(skb),
>> +
>> + TP_STRUCT__entry(
>> + __field( void *, skbaddr )
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->skbaddr = skb;
>> + ),
>> +
>> + TP_printk("skbaddr=%p", __entry->skbaddr)
>> +
>> +);
>> +
>> +DEFINE_EVENT(free_skb, consume_skb,
>> +
>> + TP_PROTO(struct sk_buff *skb),
>> +
>> + TP_ARGS(skb)
>> +
>> +);
>> +
>> +DEFINE_EVENT(free_skb, dev_kfree_skb_irq,
>> +
>> + TP_PROTO(struct sk_buff *skb),
>> +
>> + TP_ARGS(skb)
>> +
>> +);
>> +
>> +DEFINE_EVENT(free_skb, skb_free_datagram_locked,
>> +
>> + TP_PROTO(struct sk_buff *skb),
>> +
>> + TP_ARGS(skb)
>> +
>> +);
>> +
>
> Why create these last two tracepoints at all? dev_kfree_skb_irq will eventually
> pass through kfree_skb anyway, getting picked up by the tracepoint there, the
> while the latter won't (since it uses __kfree_skb instead), I think that could
> be fixed up by add a call to trace_kfree_skb there directly, saving you two
> tracepoints.
>
> Neil
>
I think dev_kfree_skb_irq isn't chased by trace_kfree_skb or trace_consume_skb
completely. Because net_tx_action frees skb by __kfree_skb. So it is better to
add trace_kfree_skb before it. skb_free_datagram_locked is same.
Thanks,
Koki Sanagi.
>>
>
>
^ permalink raw reply
* Re: [RFC PATCH v3 3/5] netdev: add tracepoints to netdev layer
From: Koki Sanagi @ 2010-07-21 7:01 UTC (permalink / raw)
To: Neil Horman
Cc: netdev, linux-kernel, davem, kaneshige.kenji, izumi.taku,
kosaki.motohiro, laijs, scott.a.mcmillan, rostedt, eric.dumazet,
fweisbec, mathieu.desnoyers
In-Reply-To: <20100720114152.GC1995@hmsreliant.think-freely.org>
(2010/07/20 20:41), Neil Horman wrote:
> On Tue, Jul 20, 2010 at 09:47:59AM +0900, Koki Sanagi wrote:
>> This patch adds tracepoint to dev_queue_xmit, dev_hard_start_xmit and
>> netif_receive_skb. These tracepoints help you to monitor network driver's
>> input/output.
>>
>> sshd-4445 [001] 241367.066046: net_dev_queue: dev=eth3 skbaddr=dd6b2538 len=114
>> sshd-4445 [001] 241367.066047: net_dev_xmit: dev=eth3 skbaddr=dd6b2538 len=114 rc=0
>> <idle>-0 [001] 241367.067472: net_dev_receive: dev=eth3 skbaddr=f5e59000 len=52
>>
>> Signed-off-by: Koki Sanagi <sanagi.koki@jp.fujitsu.com>
>> ---
>> include/trace/events/net.h | 75 ++++++++++++++++++++++++++++++++++++++++++++
>> net/core/dev.c | 5 +++
>> net/core/net-traces.c | 1 +
>> 3 files changed, 81 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/trace/events/net.h b/include/trace/events/net.h
>> new file mode 100644
>> index 0000000..8a21361
>> --- /dev/null
>> +++ b/include/trace/events/net.h
>> @@ -0,0 +1,75 @@
>> +#undef TRACE_SYSTEM
>> +#define TRACE_SYSTEM net
>> +
>> +#if !defined(_TRACE_NET_H) || defined(TRACE_HEADER_MULTI_READ)
>> +#define _TRACE_NET_H
>> +
>> +#include <linux/skbuff.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/ip.h>
>> +#include <linux/tracepoint.h>
>> +
>> +TRACE_EVENT(net_dev_xmit,
>> +
>> + TP_PROTO(struct sk_buff *skb,
>> + int rc),
>> +
>> + TP_ARGS(skb, rc),
>> +
>> + TP_STRUCT__entry(
>> + __field( void *, skbaddr )
>> + __field( unsigned int, len )
>> + __field( int, rc )
>> + __string( name, skb->dev->name )
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->skbaddr = skb;
>> + __entry->len = skb->len;
>> + __entry->rc = rc;
>> + __assign_str(name, skb->dev->name);
>> + ),
>> +
>> + TP_printk("dev=%s skbaddr=%p len=%u rc=%d",
>> + __get_str(name), __entry->skbaddr, __entry->len, __entry->rc)
>> +);
>> +
>> +DECLARE_EVENT_CLASS(net_dev_template,
>> +
>> + TP_PROTO(struct sk_buff *skb),
>> +
>> + TP_ARGS(skb),
>> +
>> + TP_STRUCT__entry(
>> + __field( void *, skbaddr )
>> + __field( unsigned int, len )
>> + __string( name, skb->dev->name )
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->skbaddr = skb;
>> + __entry->len = skb->len;
>> + __assign_str(name, skb->dev->name);
>> + ),
>> +
>> + TP_printk("dev=%s skbaddr=%p len=%u",
>> + __get_str(name), __entry->skbaddr, __entry->len)
>> +)
>> +
>> +DEFINE_EVENT(net_dev_template, net_dev_queue,
>> +
>> + TP_PROTO(struct sk_buff *skb),
>> +
>> + TP_ARGS(skb)
>> +);
>> +
>> +DEFINE_EVENT(net_dev_template, net_dev_receive,
>> +
>> + TP_PROTO(struct sk_buff *skb),
>> +
>> + TP_ARGS(skb)
>> +);
>> +#endif /* _TRACE_NET_H */
>> +
>> +/* This part must be outside protection */
>> +#include <trace/define_trace.h>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 93b8929..4acfec6 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -130,6 +130,7 @@
>> #include <linux/jhash.h>
>> #include <linux/random.h>
>> #include <trace/events/napi.h>
>> +#include <trace/events/net.h>
>> #include <linux/pci.h>
>>
>> #include "net-sysfs.h"
>> @@ -1955,6 +1956,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>> }
>>
>> rc = ops->ndo_start_xmit(skb, dev);
>> + trace_net_dev_xmit(skb, rc);
>> if (rc == NETDEV_TX_OK)
>> txq_trans_update(txq);
>> return rc;
>> @@ -1975,6 +1977,7 @@ gso:
>> skb_dst_drop(nskb);
>>
>> rc = ops->ndo_start_xmit(nskb, dev);
>> + trace_net_dev_xmit(nskb, rc);
>> if (unlikely(rc != NETDEV_TX_OK)) {
>> if (rc & ~NETDEV_TX_MASK)
>> goto out_kfree_gso_skb;
>> @@ -2165,6 +2168,7 @@ int dev_queue_xmit(struct sk_buff *skb)
>> #ifdef CONFIG_NET_CLS_ACT
>> skb->tc_verd = SET_TC_AT(skb->tc_verd, AT_EGRESS);
>> #endif
>> + trace_net_dev_queue(skb);
>> if (q->enqueue) {
>> rc = __dev_xmit_skb(skb, q, dev, txq);
>> goto out;
>> @@ -2939,6 +2943,7 @@ int netif_receive_skb(struct sk_buff *skb)
>> if (netdev_tstamp_prequeue)
>> net_timestamp_check(skb);
>>
>> + trace_net_dev_receive(skb);
>
> I imagine for completeness you'll want to make a call to this in netif_rx and
> netif_rx_ni as well.
>
It might be better to add tracepoint to netif_rx to determine a time between
netif_rx and netif_receive_skb.
Thanks,
Koki Sanagi.
>
>
>
^ permalink raw reply
* Re: [RFC PATCH v3 2/5] napi: convert trace_napi_poll to TRACE_EVENT
From: Koki Sanagi @ 2010-07-21 7:00 UTC (permalink / raw)
To: Neil Horman
Cc: netdev, linux-kernel, davem, kaneshige.kenji, izumi.taku,
kosaki.motohiro, laijs, scott.a.mcmillan, rostedt, eric.dumazet,
fweisbec, mathieu.desnoyers
In-Reply-To: <20100720110952.GB1995@hmsreliant.think-freely.org>
(2010/07/20 20:09), Neil Horman wrote:
> On Tue, Jul 20, 2010 at 09:46:51AM +0900, Koki Sanagi wrote:
>> From: Neil Horman <nhorman@tuxdriver.com>
>>
>> This patch converts trace_napi_poll from DECLARE_EVENT to TRACE_EVENT to improve
>> the usability of napi_poll tracepoint.
>>
>> <idle>-0 [001] 241302.750777: napi_poll: napi poll on napi struct f6acc480 for device eth3
>> <idle>-0 [000] 241302.852389: napi_poll: napi poll on napi struct f5d0d70c for device eth1
>>
>> An original patch is below.
>> http://marc.info/?l=linux-kernel&m=126021713809450&w=2
>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>>
>> And add a fix by Steven Rostedt.
>> http://marc.info/?l=linux-kernel&m=126150506519173&w=2
>>
>> Signed-off-by: Koki Sanagi <sanagi.koki@jp.fujitsu.com>
>> ---
>> include/trace/events/napi.h | 25 +++++++++++++++++++++++--
>> 1 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/trace/events/napi.h b/include/trace/events/napi.h
>> index 188deca..8fe1e93 100644
>> --- a/include/trace/events/napi.h
>> +++ b/include/trace/events/napi.h
>> @@ -6,10 +6,31 @@
>>
>> #include <linux/netdevice.h>
>> #include <linux/tracepoint.h>
>> +#include <linux/ftrace.h>
>> +
>> +#define NO_DEV "(no_device)"
>> +
>> +TRACE_EVENT(napi_poll,
>>
>> -DECLARE_TRACE(napi_poll,
>> TP_PROTO(struct napi_struct *napi),
>> - TP_ARGS(napi));
>> +
>> + TP_ARGS(napi),
>> +
>> + TP_STRUCT__entry(
>> + __field( struct napi_struct *, napi)
>> + __string( dev_name, napi->dev ? napi->dev->name : NO_DEV)
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->napi = napi;
>> + __assign_str(dev_name, napi->dev ? napi->dev->name : NO_DEV);
>> + ),
>> +
>> + TP_printk("napi poll on napi struct %p for device %s",
>> + __entry->napi, __get_str(dev_name))
>> +);
>> +
>> +#undef NO_DEV
>>
>> #endif /* _TRACE_NAPI_H_ */
>>
>>
> NAK, This change will create a build break in the drop monitor code. You'll
> need to fix that up if you want to make this change.
> Neil
>
I built a kernel with CONFIG_NET_DROP_MONITOR=y.
But build break did not occur.
Thanks,
Koki Sanagi.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply
* Re: [RFC PATCH v3 1/5] irq: add tracepoint to softirq_raise
From: Koki Sanagi @ 2010-07-21 6:57 UTC (permalink / raw)
To: Neil Horman
Cc: netdev, linux-kernel, davem, kaneshige.kenji, izumi.taku,
kosaki.motohiro, laijs, scott.a.mcmillan, rostedt, eric.dumazet,
fweisbec, mathieu.desnoyers
In-Reply-To: <20100720110439.GA1995@hmsreliant.think-freely.org>
(2010/07/20 20:04), Neil Horman wrote:
> On Tue, Jul 20, 2010 at 09:45:31AM +0900, Koki Sanagi wrote:
>> From: Lai Jiangshan <laijs@cn.fujitsu.com>
>>
>> Add a tracepoint for tracing when softirq action is raised.
>>
>> It and the existed tracepoints complete softirq's tracepoints:
>> softirq_raise, softirq_entry and softirq_exit.
>>
>> And when this tracepoint is used in combination with
>> the softirq_entry tracepoint we can determine
>> the softirq raise latency.
>>
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>> Acked-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
>> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
>>
>> [ factorize softirq events with DECLARE_EVENT_CLASS ]
>> Signed-off-by: Koki Sanagi <sanagi.koki@jp.fujitsu.com>
>> ---
>> include/linux/interrupt.h | 8 +++++-
>> include/trace/events/irq.h | 57 ++++++++++++++++++++++++++-----------------
>> kernel/softirq.c | 4 +-
>> 3 files changed, 43 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
>> index c233113..1cb5726 100644
>> --- a/include/linux/interrupt.h
>> +++ b/include/linux/interrupt.h
>> @@ -18,6 +18,7 @@
>> #include <asm/atomic.h>
>> #include <asm/ptrace.h>
>> #include <asm/system.h>
>> +#include <trace/events/irq.h>
>>
>> /*
>> * These correspond to the IORESOURCE_IRQ_* defines in
>> @@ -402,7 +403,12 @@ asmlinkage void do_softirq(void);
>> asmlinkage void __do_softirq(void);
>> extern void open_softirq(int nr, void (*action)(struct softirq_action *));
>> extern void softirq_init(void);
>> -#define __raise_softirq_irqoff(nr) do { or_softirq_pending(1UL << (nr)); } while (0)
>> +static inline void __raise_softirq_irqoff(unsigned int nr)
>> +{
>> + trace_softirq_raise(nr);
>> + or_softirq_pending(1UL << nr);
>> +}
>> +
> We already have tracepoints in irq_enter and irq_exit. If the goal here is to
> detect latency during packet processing, cant the delta in time between those
> two points be used to determine interrupt handling latency?
Certainly, the time between irq_entry and irq_exit is not directly related to
latency during packet processing. But it's indirectly related it.
Because softirq_entry isn't passed until irq exits and softirq_entry time is
related to packet processing latency. So I show it as a reference.
>
>
>> extern void raise_softirq_irqoff(unsigned int nr);
>> extern void raise_softirq(unsigned int nr);
>> extern void wakeup_softirqd(void);
>> diff --git a/include/trace/events/irq.h b/include/trace/events/irq.h
>> index 0e4cfb6..717744c 100644
>> --- a/include/trace/events/irq.h
>> +++ b/include/trace/events/irq.h
>> @@ -5,7 +5,9 @@
>> #define _TRACE_IRQ_H
>>
>> #include <linux/tracepoint.h>
>> -#include <linux/interrupt.h>
>> +
>> +struct irqaction;
>> +struct softirq_action;
>>
>> #define softirq_name(sirq) { sirq##_SOFTIRQ, #sirq }
>> #define show_softirq_name(val) \
>> @@ -84,56 +86,65 @@ TRACE_EVENT(irq_handler_exit,
>>
>> DECLARE_EVENT_CLASS(softirq,
>>
>> - TP_PROTO(struct softirq_action *h, struct softirq_action *vec),
>> + TP_PROTO(unsigned int nr),
>>
>> - TP_ARGS(h, vec),
>> + TP_ARGS(nr),
>>
>> TP_STRUCT__entry(
>> - __field( int, vec )
>> + __field( unsigned int, vec )
>> ),
>>
>> TP_fast_assign(
>> - __entry->vec = (int)(h - vec);
>> + __entry->vec = nr;
>> ),
>>
>> TP_printk("vec=%d [action=%s]", __entry->vec,
>> - show_softirq_name(__entry->vec))
>> + show_softirq_name(__entry->vec))
>> +);
>> +
>> +/**
>> + * softirq_raise - called immediately when a softirq is raised
>> + * @nr: softirq vector number
>> + *
>> + * Tracepoint for tracing when softirq action is raised.
>> + * Also, when used in combination with the softirq_entry tracepoint
>> + * we can determine the softirq raise latency.
>> + */
>> +DEFINE_EVENT(softirq, softirq_raise,
>> +
>> + TP_PROTO(unsigned int nr),
>> +
>> + TP_ARGS(nr)
>> );
>>
>> /**
>> * softirq_entry - called immediately before the softirq handler
>> - * @h: pointer to struct softirq_action
>> - * @vec: pointer to first struct softirq_action in softirq_vec array
>> + * @nr: softirq vector number
>> *
>> - * The @h parameter, contains a pointer to the struct softirq_action
>> - * which has a pointer to the action handler that is called. By subtracting
>> - * the @vec pointer from the @h pointer, we can determine the softirq
>> - * number. Also, when used in combination with the softirq_exit tracepoint
>> + * Tracepoint for tracing when softirq action starts.
>> + * Also, when used in combination with the softirq_exit tracepoint
>> * we can determine the softirq latency.
>> */
>> DEFINE_EVENT(softirq, softirq_entry,
>>
>> - TP_PROTO(struct softirq_action *h, struct softirq_action *vec),
>> + TP_PROTO(unsigned int nr),
>>
>> - TP_ARGS(h, vec)
>> + TP_ARGS(nr)
>> );
>>
>> /**
>> * softirq_exit - called immediately after the softirq handler returns
>> - * @h: pointer to struct softirq_action
>> - * @vec: pointer to first struct softirq_action in softirq_vec array
>> + * @nr: softirq vector number
>> *
>> - * The @h parameter contains a pointer to the struct softirq_action
>> - * that has handled the softirq. By subtracting the @vec pointer from
>> - * the @h pointer, we can determine the softirq number. Also, when used in
>> - * combination with the softirq_entry tracepoint we can determine the softirq
>> - * latency.
>> + * Tracepoint for tracing when softirq action ends.
>> + * Also, when used in combination with the softirq_entry tracepoint
>> + * we can determine the softirq latency.
>> */
>> DEFINE_EVENT(softirq, softirq_exit,
>>
>> - TP_PROTO(struct softirq_action *h, struct softirq_action *vec),
>> + TP_PROTO(unsigned int nr),
>>
>> - TP_ARGS(h, vec)
>> + TP_ARGS(nr)
>> );
>>
>> #endif /* _TRACE_IRQ_H */
>> diff --git a/kernel/softirq.c b/kernel/softirq.c
>> index 825e112..6790599 100644
>> --- a/kernel/softirq.c
>> +++ b/kernel/softirq.c
>> @@ -215,9 +215,9 @@ restart:
>> int prev_count = preempt_count();
>> kstat_incr_softirqs_this_cpu(h - softirq_vec);
>>
>> - trace_softirq_entry(h, softirq_vec);
>> + trace_softirq_entry(h - softirq_vec);
>> h->action(h);
>> - trace_softirq_exit(h, softirq_vec);
>> + trace_softirq_exit(h - softirq_vec);
>
> You're loosing information here by reducing the numbers of parameters in this
> tracepoint. How many other tracepoint scripts rely on having both pointers
> handy? Why not just do the pointer math inside your tracehook instead?
In __raise_softirq_irqoff macro there is no method to refer softirq_vec, so it
can't use softirq DECLARE_EVENT_CLASS as is.
Currently, there is no script using softirq_entry or softirq_exit.
Thanks,
Koki Sanagi.
>
>> if (unlikely(prev_count != preempt_count())) {
>> printk(KERN_ERR "huh, entered softirq %td %s %p"
>> "with preempt_count %08x,"
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
^ permalink raw reply
* Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address
From: Harald Hoyer @ 2010-07-21 6:47 UTC (permalink / raw)
To: David Miller
Cc: shemminger, bhutchings, sassmann, netdev, linux-kernel, gospo,
gregory.v.rose, alexander.h.duyck, leedom
In-Reply-To: <20100720.233457.267367495.davem@davemloft.net>
On 07/21/2010 08:34 AM, David Miller wrote:
> From: Harald Hoyer<harald@redhat.com>
> Date: Wed, 21 Jul 2010 08:26:27 +0200
>
>> On 07/20/2010 11:20 PM, David Miller wrote:
>>> From: Stephen Hemminger<shemminger@vyatta.com>
>>> Date: Tue, 20 Jul 2010 14:18:16 -0700
>>>
>>>> No one mentioned that the first octet of an Ethernet address already
>>>> indicates "software generated" Ethernet address. Per the standard,
>>>> if bit 1 is set it means address is locally assigned.
>>>>
>>>> static inline bool is_locally_assigned_ether(const u8 *addr)
>>>> {
>>>> return (addr[0]& 0x2) != 0;
>>>> }
>>>
>>> W00t!
>>>
>>> Indeed, can udev just use that? :-)
>>
>> It already does:
>> see /lib/udev/rules.d/75-persistent-net-generator.rules
>
> So... why doesn't this work?
It works.. but the information, that the MAC is randomly generated would be
valuable. So, for the non-random locally assigned MAC (with bit 1), we could
easily make persistent rules based on the MAC, instead of completely ignoring
them, like we do currently.
^ permalink raw reply
* Re: [RFC PATCH] dst: check if dst is freed in dst_check()
From: David Miller @ 2010-07-21 6:41 UTC (permalink / raw)
To: eric.dumazet; +Cc: nicolas.dichtel, netdev
In-Reply-To: <1279679288.2492.15.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 21 Jul 2010 04:28:08 +0200
> Le mardi 20 juillet 2010 à 11:49 +0200, Nicolas Dichtel a écrit :
>> diff --git a/include/net/dst.h b/include/net/dst.h
>> index 81d1413..7bf4f9a 100644
>> --- a/include/net/dst.h
>> +++ b/include/net/dst.h
>> @@ -319,6 +319,8 @@ static inline int dst_input(struct sk_buff *skb)
>>
>> static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie)
>> {
>> + if (dst->obsolete > 1)
>> + return NULL;
>> if (dst->obsolete)
>> dst = dst->ops->check(dst, cookie);
>> return dst;
>
> I believe this is not needed and redundant.
>
> In what case do you think this matters ?
>
> To my knowledge dst_check() is only used by net/xfrm/xfrm_policy.c
>
> And xfrm_dst_check() does the necessary checks.
Right, last time I was snooping around in here I came to the
same conclusion. In fact I think I'm the author of that
enormous comment in xfrm_dst_check(). :-)
^ permalink raw reply
* Re: [patch v2.6 4/4] libxt_ipvs: user-space lib for netfilter matcher xt_ipvs
From: Jan Engelhardt @ 2010-07-21 6:35 UTC (permalink / raw)
To: Simon Horman
Cc: lvs-devel, netdev, linux-kernel, netfilter, netfilter-devel,
Malcolm Turnbull, Wensong Zhang, Julius Volz, Patrick McHardy,
David S. Miller, Hannes Eder
In-Reply-To: <20100721012146.GC22966@verge.net.au>
On Wednesday 2010-07-21 03:21, Simon Horman wrote:
>> +
>> +#define XT_IPVS_IPVS_PROPERTY (1 << 0) /* all other options imply this one */
>> +#define XT_IPVS_PROTO (1 << 1)
>> +#define XT_IPVS_VADDR (1 << 2)
>> +#define XT_IPVS_VPORT (1 << 3)
>> +#define XT_IPVS_DIR (1 << 4)
>> +#define XT_IPVS_METHOD (1 << 5)
>> +#define XT_IPVS_VPORTCTL (1 << 6)
>> +#define XT_IPVS_MASK ((1 << 7) - 1)
>> +#define XT_IPVS_ONCE_MASK (XT_IPVS_MASK & ~XT_IPVS_IPVS_PROPERTY)
Can't these just be an enum?
^ permalink raw reply
* Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address
From: David Miller @ 2010-07-21 6:34 UTC (permalink / raw)
To: harald
Cc: shemminger, bhutchings, sassmann, netdev, linux-kernel, gospo,
gregory.v.rose, alexander.h.duyck, leedom
In-Reply-To: <4C469313.6010807@redhat.com>
From: Harald Hoyer <harald@redhat.com>
Date: Wed, 21 Jul 2010 08:26:27 +0200
> On 07/20/2010 11:20 PM, David Miller wrote:
>> From: Stephen Hemminger<shemminger@vyatta.com>
>> Date: Tue, 20 Jul 2010 14:18:16 -0700
>>
>>> No one mentioned that the first octet of an Ethernet address already
>>> indicates "software generated" Ethernet address. Per the standard,
>>> if bit 1 is set it means address is locally assigned.
>>>
>>> static inline bool is_locally_assigned_ether(const u8 *addr)
>>> {
>>> return (addr[0]& 0x2) != 0;
>>> }
>>
>> W00t!
>>
>> Indeed, can udev just use that? :-)
>
> It already does:
> see /lib/udev/rules.d/75-persistent-net-generator.rules
So... why doesn't this work?
^ permalink raw reply
* Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address
From: Harald Hoyer @ 2010-07-21 6:26 UTC (permalink / raw)
To: David Miller
Cc: shemminger, bhutchings, sassmann, netdev, linux-kernel, gospo,
gregory.v.rose, alexander.h.duyck, leedom
In-Reply-To: <20100720.142045.32697196.davem@davemloft.net>
On 07/20/2010 11:20 PM, David Miller wrote:
> From: Stephen Hemminger<shemminger@vyatta.com>
> Date: Tue, 20 Jul 2010 14:18:16 -0700
>
>> No one mentioned that the first octet of an Ethernet address already
>> indicates "software generated" Ethernet address. Per the standard,
>> if bit 1 is set it means address is locally assigned.
>>
>> static inline bool is_locally_assigned_ether(const u8 *addr)
>> {
>> return (addr[0]& 0x2) != 0;
>> }
>
> W00t!
>
> Indeed, can udev just use that? :-)
It already does:
see /lib/udev/rules.d/75-persistent-net-generator.rules
^ permalink raw reply
* Re: [PATCH -next] net: NET_DSA depends on NET_ETHERNET
From: Randy Dunlap @ 2010-07-21 5:41 UTC (permalink / raw)
To: David Miller; +Cc: sfr, netdev, linux-next, linux-kernel, buytenh
In-Reply-To: <20100720.174530.139530021.davem@davemloft.net>
On 07/20/10 17:45, David Miller wrote:
> From: Randy Dunlap <randy.dunlap@oracle.com>
> Date: Tue, 20 Jul 2010 16:03:32 -0700
>
>> From: Randy Dunlap <randy.dunlap@oracle.com>
>>
>> NET_DSA code selects and uses PHYLIB code, but PHYLIB depends on
>> NET_ETHERNET. However, "select" does not follow kconfig dependencies,
>> so explicitly list that requirement here instead.
>>
>> Fixes this kconfig warning:
>>
>> warning: (NET_DSA && NET && EXPERIMENTAL && !S390 ...) selects PHYLIB which has unmet direct dependencies (!S390 && NET_ETHERNET)
>>
>> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
>
> Randy, this has been fixed in net-2.6 for some time now.
OK, I did see the commit get merged today.
> And I'm pretty sure I sent a copy of this to you when I
> checked it in :-)
I missed it somehow. Thanks.
> --------------------
> From 336a283b9cbe47748ccd68fd8c5158f67cee644b Mon Sep 17 00:00:00 2001
> From: David S. Miller <davem@davemloft.net>
> Date: Mon, 12 Jul 2010 20:03:42 -0700
> Subject: [PATCH 09/24] dsa: Fix Kconfig dependencies.
>
> Based upon a report by Randy Dunlap.
>
> DSA needs PHYLIB, but PHYLIB needs NET_ETHERNET. So, in order
> to select PHYLIB we have to make DSA depend upon NET_ETHERNET.
>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
> net/dsa/Kconfig | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index c51b554..1120178 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -1,7 +1,7 @@
> menuconfig NET_DSA
> bool "Distributed Switch Architecture support"
> default n
> - depends on EXPERIMENTAL && !S390
> + depends on EXPERIMENTAL && NET_ETHERNET && !S390
> select PHYLIB
> ---help---
> This allows you to use hardware switch chips that use
--
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply
* Re: [patch 1/1] drivers/s390/net: use memdup_user
From: Frank Blaschka @ 2010-07-21 5:35 UTC (permalink / raw)
To: akpm; +Cc: netdev, linux-s390
In-Reply-To: <201007202227.o6KMRZ7V021566@imap1.linux-foundation.org>
Hi,
I added this patch to my patch set
Thx
Frank
^ permalink raw reply
* [PATCH 2/2] sysfs: allow creating symlinks from untagged to tagged directories
From: Eric W. Biederman @ 2010-07-21 5:12 UTC (permalink / raw)
To: Greg KH
Cc: Andrew Morton, Greg KH, Rafael J. Wysocki, Maciej W. Rozycki,
Kay Sievers, Johannes Berg, netdev
In-Reply-To: <m1vd894dy5.fsf_-_@fess.ebiederm.org>
Supporting symlinks from untagged to tagged directories is reasonable,
and needed to support CONFIG_SYSFS_DEPRECATED. So don't fail a prior
allowing that case to work.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/sysfs/symlink.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 6603833..a7ac78f 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -67,7 +67,8 @@ static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
sysfs_addrm_start(&acxt, parent_sd);
/* Symlinks must be between directories with the same ns_type */
- if (ns_type == sysfs_ns_type(sd->s_symlink.target_sd->s_parent)) {
+ if (!ns_type ||
+ (ns_type == sysfs_ns_type(sd->s_symlink.target_sd->s_parent))) {
if (warn)
error = sysfs_add_one(&acxt, sd);
else
--
1.6.5.2.143.g8cc62
^ permalink raw reply related
* [PATCH 1/2] sysfs: sysfs_delete_link handle symlinks from untagged to tagged directories.
From: Eric W. Biederman @ 2010-07-21 5:10 UTC (permalink / raw)
To: Greg KH
Cc: Andrew Morton, Greg KH, Rafael J. Wysocki, Maciej W. Rozycki,
Kay Sievers, Johannes Berg, netdev
In-Reply-To: <m139vd5sms.fsf_-_@fess.ebiederm.org>
This happens for network devices when SYSFS_DEPRECATED is enabled.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/sysfs/symlink.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 44bca5f..6603833 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -135,7 +135,7 @@ void sysfs_delete_link(struct kobject *kobj, struct kobject *targ,
{
const void *ns = NULL;
spin_lock(&sysfs_assoc_lock);
- if (targ->sd)
+ if (targ->sd && sysfs_ns_type(kobj->sd))
ns = targ->sd->s_ns;
spin_unlock(&sysfs_assoc_lock);
sysfs_hash_and_remove(kobj->sd, ns, name);
--
1.6.5.2.143.g8cc62
^ permalink raw reply related
* [PATCH 0/2] Support untagged symlinks to tagged directories.
From: Eric W. Biederman @ 2010-07-21 5:08 UTC (permalink / raw)
To: Greg KH
Cc: Andrew Morton, Greg KH, Rafael J. Wysocki, Maciej W. Rozycki,
Kay Sievers, Johannes Berg, netdev
In-Reply-To: <20100720201334.GA11991@suse.de>
Greg KH <gregkh@suse.de> writes:
> On Mon, Jul 19, 2010 at 01:34:51PM -0700, Andrew Morton wrote:
>> On Thu, 8 Jul 2010 16:06:01 -0700
>> Greg KH <greg@kroah.com> wrote:
>>
>> > On Thu, Jul 08, 2010 at 03:28:53PM -0700, Eric W. Biederman wrote:
>> > > Greg KH <greg@kroah.com> writes:
>> > >
>> > > > With this patch, how does the existing code fail as the drivers aren't
>> > > > fixed up?
>> > > >
>> > > > I like this change, just worried it will cause problems if it gets into
>> > > > .35, without your RFC patch. Will it?
>> > >
>>
>> geethanks!
>>
>> On the FC6 test box I have no networking.
>
> Ick.
>
> Eric, any ideas?
Yes. I just found some time to test my fixes and things are looking
good. It really is just two one line fixes.
On the other part of this debug with SYSFS_DEPRECATED enabled it
with mac80211_hwsim drivers works fine no problems. I expect the
bnep driver will also be fine.
What is affecting those two is arguably a bug in the non-deprecated
sysfs mode.
Regardless here are my fixes. I have split this into a patch for
the warning and a patch for sysfs_delete_link. Because at least
the sysfs_delete_link code needs to make into 2.6.35 if we can.
Eric
^ permalink raw reply
* Re: [PATCH] Export SMBIOS provided firmware instance and label to sysfs
From: Greg KH @ 2010-07-21 3:59 UTC (permalink / raw)
To: Narendra K
Cc: netdev, linux-hotplug, linux-pci, matt_domsch, charles_rose,
jordan_hargrave, vijay_nijhawan
In-Reply-To: <20100714121345.GA20411@auslistsprd01.us.dell.com>
On Wed, Jul 14, 2010 at 07:13:45AM -0500, Narendra K wrote:
> @@ -333,6 +358,7 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
> break;
> case 41: /* Onboard Devices Extended Information */
> dmi_save_extended_devices(dm);
> + break;
Why make this change? It's not relevant to your patch, right?
> +enum smbios_attr_enum {
> + SMBIOS_ATTR_NONE = 0,
> + SMBIOS_ATTR_LABEL_SHOW,
> + SMBIOS_ATTR_INSTANCE_SHOW,
> +};
> +
> +static mode_t
> +find_smbios_instance_string(struct pci_dev *pdev, char *buf, int attribute)
Why isn't 'attribute' an enumerated type like you just defined above?
Extra type-checking is always good, especially as the variable name
'attribute' means something totally different in other parts of this
file.
> +{
> + const struct dmi_device *dmi;
> + struct dmi_dev_onboard *donboard;
> + int bus;
> + int devfn;
> +
> + bus = pdev->bus->number;
> + devfn = pdev->devfn;
> +
> + dmi = NULL;
> + while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_ONBOARD,
> + NULL, dmi)) != NULL) {
> + donboard = dmi->device_data;
> + if (donboard && donboard->bus == bus &&
> + donboard->devfn == devfn) {
> + if (buf) {
> + if (attribute == SMBIOS_ATTR_INSTANCE_SHOW)
> + return scnprintf(buf, PAGE_SIZE,
> + "%d\n",
> + donboard->instance);
> + else if (attribute == SMBIOS_ATTR_LABEL_SHOW)
> + return scnprintf(buf, PAGE_SIZE,
> + "%s\n",
> + dmi->name);
> + }
> + return strlen(dmi->name);
> + }
> + }
> + return 0;
> +}
> +
> +static mode_t
> +smbios_instance_string_exist(struct kobject *kobj, struct attribute *attr,
> + int n)
> +{
> + struct device *dev;
> + struct pci_dev *pdev;
> +
> + dev = container_of(kobj, struct device, kobj);
> + pdev = to_pci_dev(dev);
> +
> + return find_smbios_instance_string(pdev, NULL, SMBIOS_ATTR_NONE);
> +}
> +
> +static ssize_t
> +smbioslabel_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct pci_dev *pdev;
> + pdev = to_pci_dev(dev);
> +
> + return find_smbios_instance_string(pdev, buf,
> + SMBIOS_ATTR_LABEL_SHOW);
> +}
> +
> +static ssize_t
> +smbiosinstance_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct pci_dev *pdev;
> + pdev = to_pci_dev(dev);
> +
> + return find_smbios_instance_string(pdev, buf,
> + SMBIOS_ATTR_INSTANCE_SHOW);
> +}
> +
> +static struct device_attribute smbios_attr_label = {
> + .attr = {.name = "label", .mode = 0444, .owner = THIS_MODULE},
> + .show = smbioslabel_show,
> +};
> +
> +static struct device_attribute smbios_attr_instance = {
> + .attr = {.name = "index", .mode = 0444, .owner = THIS_MODULE},
> + .show = smbiosinstance_show,
> +};
> +
> +static struct attribute *smbios_attributes[] = {
> + &smbios_attr_label.attr,
> + &smbios_attr_instance.attr,
> + NULL,
> +};
> +
> +static struct attribute_group smbios_attr_group = {
> + .attrs = smbios_attributes,
> + .is_visible = smbios_instance_string_exist,
> +};
> +
> +static int
> +pci_create_smbiosname_file(struct pci_dev *pdev)
> +{
> + if (!sysfs_create_group(&pdev->dev.kobj, &smbios_attr_group))
> + return 0;
> + return -ENODEV;
> +}
> +
> +static int
> +pci_remove_smbiosname_file(struct pci_dev *pdev)
> +{
> + sysfs_remove_group(&pdev->dev.kobj, &smbios_attr_group);
> + return 0;
> +}
What's with the extra indentation?
Why return a value at all here?
> +
> +int pci_create_firmware_label_files(struct pci_dev *pdev)
> +{
> + if (!pci_create_smbiosname_file(pdev))
> + return 0;
> + return -ENODEV;
> +}
> +
> +int pci_remove_firmware_label_files(struct pci_dev *pdev)
> +{
> + if (!pci_remove_smbiosname_file(pdev))
> + return 0;
> + return -ENODEV;
> +}
Why return values for these two functions if you never check them
anywhere? Either check the return value and do something with it, or
just make them 'void'.
Also, you need to add documentation for what this sysfs file is and does
in the Documentation/ABI/ directory. That must be in this patch to have
it acceptable.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] Export SMBIOS provided firmware instance and label to sysfs
From: Greg KH @ 2010-07-21 3:54 UTC (permalink / raw)
To: Narendra_K
Cc: netdev, linux-hotplug, linux-pci, Matt_Domsch, Charles_Rose,
Jordan_Hargrave, Vijay_Nijhawan
In-Reply-To: <EDA0A4495861324DA2618B4C45DCB3EE612BD1@blrx3m08.blr.amer.dell.com>
On Mon, Jul 19, 2010 at 10:24:39PM +0530, Narendra_K@Dell.com wrote:
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-
> > owner@vger.kernel.org] On Behalf Of Narendra K
> > Sent: Wednesday, July 14, 2010 5:44 PM
> > To: greg@kroah.com
> > Cc: netdev@vger.kernel.org; linux-hotplug@vger.kernel.org; linux-
> > pci@vger.kernel.org; Domsch, Matt; Rose, Charles; Hargrave, Jordan;
> > Nijhawan, Vijay
> > Subject: Re: [PATCH] Export SMBIOS provided firmware instance and
> label
> > to sysfs
> >
> >
> > V1 -> V2:
> >
> > 1. The 'smbios_attr' buffer is not being used as mentioned above
> >
> > 2. The function 'smbios_instance_string_exist' is split into two
> > functions,
> > the other being 'find_smbios_instance_string' which would print the
> > result
> > into the sysfs provided 'buf' of associated device. The function
> > 'smbios_instance_string_exist' would let us know if the label exists
> or
> > not.
> >
> > Please find the patch with above changes here -
> >
> > From: Narendra K <narendra_k@dell.com>
> > Subject: [PATCH] Export SMBIOS provided firmware instance and label to
> > sysfs
> >
>
> Greg,
>
> Thanks for the review comments.
>
> This version of the patch has all the suggestions incorporated. Please
> let us know if there are any concerns. If the approach is acceptable,
> please consider this patch for inclusion.
What "version"? The previous one you sent? I'll look at it, but note
that I'm not the maintainer who you need to convince to accept it :)
thanks,
greg k-h
^ permalink raw reply
* Re: linux-next: manual merge of the net tree with Linus' tree
From: Stephen Rothwell @ 2010-07-21 3:44 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-next, linux-kernel, herbert
In-Reply-To: <20100720.202759.00438343.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 500 bytes --]
Hi Dave,
On Tue, 20 Jul 2010 20:27:59 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
>
> The net-2.6 changes should be undone as net-next-2.6 has the fixes
> that allow bridge netpoll to work properly, thus in net-next-2.6 the
> net-2.6 commit is completely unnecessary.
>
> I did this when I merged net-2.6 into net-next-2.6 about an hour ago
> :-)
Excellent, thanks.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: linux-next: manual merge of the net tree with Linus' tree
From: David Miller @ 2010-07-21 3:27 UTC (permalink / raw)
To: sfr; +Cc: netdev, linux-next, linux-kernel, herbert
In-Reply-To: <20100721120448.31e325fd.sfr@canb.auug.org.au>
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Wed, 21 Jul 2010 12:04:48 +1000
> Today's linux-next merge of the net tree got a conflict in
> net/bridge/br_device.c between commit
> 573201f36fd9c7c6d5218cdcd9948cee700b277d ("bridge: Partially disable
> netpoll support") from Linus' tree and commit
> 91d2c34a4eed32876ca333b0ca44f3bc56645805 ("bridge: Fix netpoll support")
> from the net tree.
>
> The net tree commit seems to be a fuller fix, so I used that.
The net-2.6 changes should be undone as net-next-2.6 has the fixes
that allow bridge netpoll to work properly, thus in net-next-2.6 the
net-2.6 commit is completely unnecessary.
I did this when I merged net-2.6 into net-next-2.6 about an hour ago
:-)
^ permalink raw reply
* Re: [PATCH net-next-2.6] ixgbe: fix ethtool stats
From: Eric Dumazet @ 2010-07-21 2:38 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: David Miller, Jesse Brandeburg, PJ Waskiewicz, netdev
In-Reply-To: <AANLkTik7JrI3HrtvQRTgrRU30fFb2lrgGxUJsWXedBL0@mail.gmail.com>
Le mardi 20 juillet 2010 à 15:06 -0700, Jeff Kirsher a écrit :
> On Tue, Jul 20, 2010 at 10:28, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Note : I am currently unable to test following patch, could you please
> > Intel guys test it and Ack (or Nack) it ?
> >
> > Thanks !
> >
> > [PATCH net-next-2.6] ixgbe: fix ethtool stats
> >
> > In latest changes about 64bit stats on 32bit arches,
> > [commit 28172739f0a276eb8 (net: fix 64 bit counters on 32 bit arches)],
> > I missed ixgbe uses a bit of magic in its ixgbe_gstrings_stats
> > definition.
> >
> > IXGBE_NETDEV_STAT() must now assume offsets relative to
> > rtnl_link_stats64, not relative do dev->stats.
> >
> > As a bonus, we also get 64bit stats on ethtool -S
> >
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > ---
> > drivers/net/ixgbe/ixgbe_ethtool.c | 42 ++++++++++++++--------------
> > 1 file changed, 21 insertions(+), 21 deletions(-)
> >
>
> Thanks Eric, I have added it to my queue.
>
Thanks !
By the way, my ixgbe conf doesnt like net-next-2.6 at all.
(No link is established in my fiber loop configuration)
current linux-2.6 git runs correctly, link at 10Gb, so there is a
regression somewhere.
As this machine is quite slow (I dont have anymore my Nehalem dev
machine, had to use an old setup), a bisection would take one month...
^ permalink raw reply
* Re: linux-next: manual merge of the net tree with Linus' tree
From: Herbert Xu @ 2010-07-21 2:31 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: David Miller, netdev, linux-next, linux-kernel
In-Reply-To: <20100721120448.31e325fd.sfr@canb.auug.org.au>
On Wed, Jul 21, 2010 at 12:04:48PM +1000, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the net tree got a conflict in
> net/bridge/br_device.c between commit
> 573201f36fd9c7c6d5218cdcd9948cee700b277d ("bridge: Partially disable
> netpoll support") from Linus' tree and commit
> 91d2c34a4eed32876ca333b0ca44f3bc56645805 ("bridge: Fix netpoll support")
> from the net tree.
>
> The net tree commit seems to be a fuller fix, so I used that.
Yeah, 573201f is just the temporary fix for 2.6.35.
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [RFC PATCH] dst: check if dst is freed in dst_check()
From: Eric Dumazet @ 2010-07-21 2:28 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev
In-Reply-To: <4C457120.9070105@6wind.com>
Le mardi 20 juillet 2010 à 11:49 +0200, Nicolas Dichtel a écrit :
> Hi,
>
> I probably missed something, but I cannot find where obsolete field is checked
> when dst_check() is called. If dst->obsolete is > 1, dst cannot be used!
>
> Attached is a proposal to fix this issue.
>
>
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 81d1413..7bf4f9a 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -319,6 +319,8 @@ static inline int dst_input(struct sk_buff *skb)
>
> static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie)
> {
> + if (dst->obsolete > 1)
> + return NULL;
> if (dst->obsolete)
> dst = dst->ops->check(dst, cookie);
> return dst;
I believe this is not needed and redundant.
In what case do you think this matters ?
To my knowledge dst_check() is only used by net/xfrm/xfrm_policy.c
And xfrm_dst_check() does the necessary checks.
static struct dst_entry *xfrm_dst_check(struct dst_entry *dst, u32 cookie)
{
/* Code (such as __xfrm4_bundle_create()) sets dst->obsolete
* to "-1" to force all XFRM destinations to get validated by
* dst_ops->check on every use. We do this because when a
* normal route referenced by an XFRM dst is obsoleted we do
* not go looking around for all parent referencing XFRM dsts
* so that we can invalidate them. It is just too much work.
* Instead we make the checks here on every use. For example:
*
* XFRM dst A --> IPv4 dst X
*
* X is the "xdst->route" of A (X is also the "dst->path" of A
* in this example). If X is marked obsolete, "A" will not
* notice. That's what we are validating here via the
* stale_bundle() check.
*
* When a policy's bundle is pruned, we dst_free() the XFRM
* dst which causes it's ->obsolete field to be set to a
* positive non-zero integer. If an XFRM dst has been pruned
* like this, we want to force a new route lookup.
*/
if (dst->obsolete < 0 && !stale_bundle(dst))
return dst;
return NULL;
}
^ permalink raw reply
* linux-next: manual merge of the net tree with Linus' tree
From: Stephen Rothwell @ 2010-07-21 2:04 UTC (permalink / raw)
To: David Miller, netdev; +Cc: linux-next, linux-kernel, Herbert Xu
[-- Attachment #1: Type: text/plain, Size: 493 bytes --]
Hi all,
Today's linux-next merge of the net tree got a conflict in
net/bridge/br_device.c between commit
573201f36fd9c7c6d5218cdcd9948cee700b277d ("bridge: Partially disable
netpoll support") from Linus' tree and commit
91d2c34a4eed32876ca333b0ca44f3bc56645805 ("bridge: Fix netpoll support")
from the net tree.
The net tree commit seems to be a fuller fix, so I used that.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* linux-next: manual merge of the wireless tree with the net tree
From: Stephen Rothwell @ 2010-07-21 2:04 UTC (permalink / raw)
To: John W. Linville
Cc: linux-next, linux-kernel, Eric Dumazet, Wey-Yi Guy, David Miller,
netdev
Hi John,
Today's linux-next merge of the wireless tree got a conflict in
drivers/net/wireless/iwlwifi/iwl-commands.h between commit
ba2d3587912f82d1ab4367975b1df460db60fb1e ("drivers/net: use __packed
annotation") from the net tree and commit
7c094c5cc4d28062abf0d33ca022dbea6c522558 ("iwlwifi: additional statistic
debug counter") from the wireless tree.
I fixed it up (see below) and can carry the fix as necessary.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
diff --cc drivers/net/wireless/iwlwifi/iwl-commands.h
index 8d2db9d,83247f7..0000000
--- a/drivers/net/wireless/iwlwifi/iwl-commands.h
+++ b/drivers/net/wireless/iwlwifi/iwl-commands.h
@@@ -3035,8 -3035,9 +3035,9 @@@ struct iwl39_statistics_tx
struct statistics_dbg {
__le32 burst_check;
__le32 burst_count;
- __le32 reserved[4];
+ __le32 wait_for_silence_timeout_cnt;
+ __le32 reserved[3];
-} __attribute__ ((packed));
+} __packed;
struct iwl39_statistics_div {
__le32 tx_on_a;
^ permalink raw reply
* Re: [PATCH] LSM: Add post accept() hook.
From: Tetsuo Handa @ 2010-07-21 2:00 UTC (permalink / raw)
To: paul.moore
Cc: davem, eric.dumazet, jmorris, sam, serge, netdev,
linux-security-module
In-Reply-To: <201007201552.29539.paul.moore@hp.com>
Paul Moore wrote:
> On Monday, July 19, 2010 09:36:31 pm Tetsuo Handa wrote:
> > One is for dropping connections from unwanted hosts. Administrators define
> > policy before enabling enforcing mode (the mode which connections are
> > dropped if operation was not granted by policy). Administrators specify
> > acceptable hosts (i.e. hosts which this host needs to communicate with)
> > and unacceptable hosts (i.e. hosts which this host needn't to communicate
> > with).
>
> You can enforce per-host access controls without the need for a post-accept()
> hooks, e.g. security_sock_rcv_skb() and the netfilter hooks
> (NF_INET_POST_ROUTING, NF_INET_FORWARD, NF_INET_LOCAL_OUT). Or are you
> interested in controlling which hosts an _application_ can communicate with?
I'm interested in controlling which ports on which hosts a _process_ can
communicate with. In TOMOYO's words, "processes that belong to which TOMOYO's
domain can communicate with which ports on which hosts".
TOMOYO's rules are
Processes that belong to FOO domain can open /etc/fstab for reading.
( allow_read /etc/fstab )
Processes that belong to FOO domain can create /tmp/file with mode 0600.
( allow_create /tmp/file 0600 )
Processes that belong to FOO domain can connect to port 80 on host
10.20.30.40 using TCP protocol.
( allow_network TCP connect 10.20.30.40 80 )
and so on. But currently,
Processes that belong to FOO domain can accept TCP connections from port 1024
on host 10.20.30.40.
( allow_network TCP accept 10.20.30.40 1024 )
Processes that belong to FOO domain can receive UDP messages from port 65535
on host 100.200.10.20.
( allow_network UDP connect 100.200.10.20 65535 )
are impossible.
Regarding outgoing connections/datagrams, we can specify address/port
parameters from the point of view of _process_ who actually sends requests.
But regarding incoming connections/datagrams, we cannot specify address/port
parameters from the point of view of _process_ who actually receives requests.
We can enforce per-host access controls using iptables.
But we can't use iptables for controlling address/port parameters for incoming
connections/datagrams because the process who actually receives requests
(ServewrApp2 in below example) is not always the same as the process who
created the socket (ServerApp1 in below example).
> > Dropping connections would happen if some process was hijacked and the
> > process attempted to communicate with other processes using TCP
> > connections. But dropping connections should not happen in normal
> > circumstance.
>
> It doesn't matter if dropping connections is normal or not, what matters is
> that it can happen.
>
> > The other is for updating process's state variable upon accept() operation.
> > LKM version of TOMOYO has per a task_struct variable that is used for
> > implementing stateful permissions. (As of now, not implemented for LSM
> > version of TOMOYO.)
>
> I'm open to re-introducing a post-accept() hook that does not have a return
> value, in other words, a hook that can only be used to update LSM state and
> not affect the connection. Although I do think you could probably achieve the
> same thing using some of the existing LSM hooks (look at how SELinux updates
> its state upon accept()) but that is something you would have to look it and
> see if it works for TOMOYO.
I can't figure out why the hook must not affect the connection.
Is it possible to clarify using below players?
Server1 and Client1 are hosts which are connected on TCP/IP network.
ServerApp1 and ServerApp2 are applications running on Server1 which might call
socket(), bind(), listen(), accept(), send(), recv(), shutdown(), close() and
execute().
ClientApp1 and ClientApp2 are applications running on Client1 which might call
socket(), connect(), send(), recv(), shutdown(), close().
Router1 and Router2 are routers which exist between Server1 and Client1.
+-------+ +-------+ +-------+ +-------+
|Server1|---|Router1|---|Router2|---|Client1|
+-------+ +-------+ +-------+ +-------+
Event sequences:
Server1 Client1
ServerApp1 creates a socket using socket().
ServerApp1 binds to an address using bind().
ServerApp1 listens to the address using listen().
ClientApp1 creates a socket using socket().
ClientApp1 issues connect() request.
Sends SYN.
Receives SYN.
Sends SYN/ACK.
Receives SYN/ACK.
Sends ACK.
Receives ACK.
ClientApp1 issues send() request.
Sends data.
Receives data.
Sends ACK.
Receives ACK.
ClientApp1 issues send() request.
Sends data.
Receives data.
Sends ACK.
Receives ACK.
ServerApp1 calls execve("ServerApp2").
ServerApp2 issues accept() request.
security_socket_accept() is called.
sock->ops->accept() is called.
security_socket_post_accept() is called. (*3)
newsock->ops->getname() is called. (*1)
move_addr_to_user() is called. (*2)
fd_install() is called.
ServerApp2 issues some requests.
Some LSM hooks will be called.
*1: This may fail and the connection is discarded if failed.
Thus, newsock->ops->getname() affects the connection.
This is not fault of ServerApp2. Maybe this is fault of ClientApp1 or
Router1 or Router2, but discarding already established connection is
justified.
*2: This may fail and the connection is discarded if failed.
Thus, move_addr_to_user() affects the connection.
Is this the fault of ServerApp2?
If the upeer_sockaddr supplied by ServerApp2 was bad, this is the fault of
ServerApp2. Thus, discarding already established connection is justified.
If the upeer_sockaddr supplied by ServerApp2 was good but physical RAM was
not yet assigned for the upeer_sockaddr, and OOM killer was invoked when
attempted to write to upeer_sockaddr and OOM killer chose ServerApp2, and
the ServerApp2 is killed. This is not fault of ServerApp2. But discarding
already established connection is justified.
*3: newsock->ops->getname() and move_addr_to_user() already affects the
connection. They discard already established connections even if the cause
is not ServerApp2's fault. Why security_socket_post_accept() affecting the
connection cannot be justified?
Router1 and Router2 can inject RST into the already established connections
at any time (if they are IDS/IPS or broken or malicious).
How does security_socket_post_accept() returning an error differs from these
routers injecting RST?
Regards.
^ permalink raw reply
* Re: linux-next: manual merge of the net tree with the net-current tree
From: David Miller @ 2010-07-21 1:27 UTC (permalink / raw)
To: joe; +Cc: sfr, netdev, linux-next, linux-kernel, jdike, mst
In-Reply-To: <1279593240.19374.2.camel@Joe-Laptop.home>
From: Joe Perches <joe@perches.com>
Date: Mon, 19 Jul 2010 19:34:00 -0700
> On Tue, 2010-07-20 at 12:20 +1000, Stephen Rothwell wrote:
>> I fixed it up (see below) and can carry the fix as necessary.
> @@@ -527,15 -527,12 +527,14 @@@ static long vhost_net_set_backend(struc
>
> /* start polling new socket */
> oldsock = vq->private_data;
> - if (sock == oldsock)
> - goto done;
> + if (sock != oldsock){
>
> Trivial: missing space before open brace in commit
> dd1f4078f0d2de74a308f00a2dffbd550cfba59f
Thanks guys, I'm taking care of this as I merge net-2.6 into
net-next-2.6
^ 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