* Re: [NET]: Add netif_tx_lock
From: Herbert Xu @ 2006-06-06 10:22 UTC (permalink / raw)
To: David Miller; +Cc: mchan, jgarzik, netdev
In-Reply-To: <20060605.213250.85688925.davem@davemloft.net>
On Mon, Jun 05, 2006 at 09:32:50PM -0700, David Miller wrote:
>
> IPOIB is going to BUG() with this change. Because now, in their
> multicast code, you're going to local_bh_disable() via
> netif_tx_unlock() with hw IRQs disabled which is illegal.
>
> It shows a bug here in the locking of the IPOIB driver.
You had me woried there.
> We need to think about this change some more. :)
I thought about it a bit more and I'm not worried anymore :)
Notice that the patch does netif_tx_lock/netif_tx_unlock
for IB instead of netif_tx_lock_bh/netif_tx_unlock_bh.
So there is no BH enabling going on at all.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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: [PATCH 2.6.18 1/3] tg3: Remove unnecessary tx_lock
From: Herbert Xu @ 2006-06-06 10:17 UTC (permalink / raw)
To: Michael Chan; +Cc: davem, jgarzik, netdev
In-Reply-To: <1551EAE59135BE47B544934E30FC4FC041BCBA@NT-IRVA-0751.brcm.ad.broadcom.com>
On Mon, Jun 05, 2006 at 10:10:56PM -0700, Michael Chan wrote:
>
> In places where we don't call tg3_netif_stop() before tg3_full_lock(),
> we are typically doing one of the following:
>
> - changing tg3_flags or tg3_flags2
> - reprogramming MAC address
> - reprogramming interrupt coalescing values
> - reprogramming the PHY
> - setting the rx mode
OK, thanks for checking.
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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: 2.6.17-rc5-mm3
From: Mel Gorman @ 2006-06-06 9:43 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, netdev
In-Reply-To: <20060605115456.c2b1e156.akpm@osdl.org>
On Mon, 5 Jun 2006, Andrew Morton wrote:
> On Mon, 5 Jun 2006 18:56:37 +0100
> mel@csn.ul.ie (Mel Gorman) wrote:
>
>>
>> I am seeing more networking-related funniness with 2.6.17-rc5-mm3 on the
>> same machine previously fixed by git-net-llc-fix.patch. The console log is
>> below. I've done no investigation work in case it's a known problem.
>
> It's not a known problem, afaik.
>
>> ...
>> Starting anacron: [ OK ]
>> Starting atd: [ OK ]
>> Starting Avahi daemon: [ OK ]
>> Starting cups-config-daemon: [ OK ]
>> Starting HAL daemon: [ OK ]
>> Fedora Core release 5 (Bordeaux)
>> Kernel 2.6.17-rc5-mm2-autokern1 on an x86_64
>> bl6-13.ltc.austin.ibm.com login: -- 0:conmux-control -- time-stamp -- Jun/05/06 10:47:46 --
>> -- 0:conmux-control -- time-stamp -- Jun/05/06 10:51:12 --
>> BUG: warning at include/net/dst.h:153/dst_release()
>> Call Trace:
>> <IRQ> [<ffffffff81228274>] __kfree_skb+0x3c/0xbd
>> [<ffffffff81199568>] tg3_poll+0x1a1/0x94f
>> [<ffffffff8122d80c>] net_rx_action+0xac/0x160
>> [<ffffffff81037904>] __do_softirq+0x48/0xb4
>> [<ffffffff8100a496>] call_softirq+0x1e/0x28
>> [<ffffffff8100b84e>] do_softirq+0x2c/0x7e
>> [<ffffffff8100b6c8>] do_IRQ+0x50/0x59
>> [<ffffffff81007807>] default_idle+0x0/0x54
>> [<ffffffff810097b8>] ret_from_intr+0x0/0xb
>> <EOI>
>> Attempt to release alive inet socket ffff81003f8b2780
>> BUG: warning at include/net/dst.h:153/dst_release()
>> Call Trace:
>> <IRQ> [<ffffffff81228274>] __kfree_skb+0x3c/0xbd
>> [<ffffffff81268fc4>] icmp_rcv+0x17c/0x184
>> [<ffffffff812484ca>] ip_local_deliver+0xfe/0x1bf
>> [<ffffffff812489bf>] ip_rcv+0x434/0x475
>> [<ffffffff8122d615>] netif_receive_skb+0x2c6/0x2e5
>> [<ffffffff81199add>] tg3_poll+0x716/0x94f
>> [<ffffffff8122d80c>] net_rx_action+0xac/0x160<7>Losing some ticks... checking if CPU frequency changed.
>> [<ffffffff81037904>] __do_softirq+0x48/0xb4
>> [<ffffffff8100a496>] call_softirq+0x1e/0x28
>> [<ffffffff8100b84e>] do_softirq+0x2c/0x7e
>> [<ffffffff8100b6c8>] do_IRQ+0x50/0x59
>> [<ffffffff81007807>] default_idle+0x0/0x54
>> [<ffffffff810097b8>] ret_from_intr+0x0/0xb
>
> There are quite a few changes in the net tree. I guess the first thing to
> investigate would be 2.6.17-rc5+origin.patch+git-net.patch.
>
That survived long enough to build a kernel, but backing out git-net on
top of mm like I did for the LLC bug also survived. Not sure what is going
on.
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
^ permalink raw reply
* Re: NETDEV WATCHDOG: eth2: transmit timed out with 3c905C-TX
From: Marco Berizzi @ 2006-06-06 9:12 UTC (permalink / raw)
To: klassert; +Cc: netdev
In-Reply-To: <20060523205842.GC18008@bayes.mathematik.tu-chemnitz.de>
Steffen Klassert wrote:
>On Tue, May 23, 2006 at 03:36:35PM +0200, Marco Berizzi wrote:
> > Steffen Klassert wrote:
> >
> > >On Wed, Apr 05, 2006 at 06:33:18PM +0200, Marco Berizzi wrote:
> > >> Hello everybody.
> > >> I'm getting these errors (with packet/connectivity loss) on
> > >> our firewall after I have plugged in a 3c905C nic. Linux is
> > >> Slackware 10.2 with vanilla 2.6.16.1.
> > >>
> > >> Hints?
> > >>
> > >> PS: I have temporary resolved the problem running 'ifconfig
> > >> eth2 down' and 'ifconfig eth2 up'
> > >>
> > >> Apr 5 17:47:07 Teti kernel: eth2: Resetting the Tx ring pointer.
> > >> Apr 5 17:47:47 Teti last message repeated 4 times
> > >> Apr 5 17:48:57 Teti last message repeated 7 times
> > >> Apr 5 17:49:57 Teti last message repeated 6 times
> > >> Apr 5 17:50:57 Teti last message repeated 6 times
> > >>
> > >> Apr 5 17:47:07 Teti kernel: NETDEV WATCHDOG: eth2: transmit timed
>out
> > >
> > >There were some problems of this kind with 10base2 networks in 2.6.16.
> > >Could you please try whether 2.6.17-rc1 has this problems too?
> >
> > [Sorry for the very huge delay, but after 2.6.17-rc1 upgrade
> > xfs filesystem crashed].
> >
> > Same problem here with 2.6.17-rc3-git18. Running ifconfig
> > eth2 down and ifconfig eth2 up resolves the problem for
> > a while.
>
>Actually I have not really an idea what is going on here,
>but increasing the debug level could give some more informations.
>Setting debug=4 is a good start.
I have moved this damn pc from the remote to my site and I have
placed it in production environment with 2.6.17-rc5
No problem after 24 hours (on the remote side the problem was
arising after a couple of hours). I have modprobed 3c59x with
debug=4. I see only these kind of messages (are they fine?):
Jun 5 14:31:25 Pleiadi kernel: eth2: vortex_error(), status=0x8081
Jun 5 14:31:40 Pleiadi last message repeated 3 times
Jun 5 14:31:47 Pleiadi kernel: eth2: vortex_error(), status=0x8281
Jun 5 14:31:47 Pleiadi kernel: eth2: Media selection timer tick happened,
Autonegotiate.
Jun 5 14:31:47 Pleiadi kernel: dev->watchdog_timeo=1250
Jun 5 14:31:47 Pleiadi kernel: eth2: MII transceiver has status 782d.
Jun 5 14:31:47 Pleiadi kernel: eth2: Media selection timer finished,
Autonegotiate.
Jun 5 14:31:51 Pleiadi kernel: eth2: vortex_error(), status=0x8081
Jun 5 14:32:03 Pleiadi last message repeated 2 times
Jun 5 14:32:10 Pleiadi kernel: eth2: vortex_error(), status=0x8481
Jun 5 14:32:15 Pleiadi kernel: eth2: vortex_error(), status=0x8081
Jun 5 14:32:46 Pleiadi last message repeated 7 times
The only relevant change, between the remote and my site, is a
different ethernet switch where the 3c905C is connected to.
Could it be an issue?
>Did you try older kernel versions too?
I started with 2.6.16.x because I need iptables policy match.
^ permalink raw reply
* Re: [PATCH 2/3] pci: bcm43xx avoid pci_find_device
From: Greg KH @ 2006-06-06 6:50 UTC (permalink / raw)
To: Jeff Garzik
Cc: Greg KH, Jiri Slaby, Linux Kernel Mailing List, linux-pci,
jgarzik, netdev, mb, st3, linville
In-Reply-To: <20060606011818.GA4135@havoc.gtf.org>
On Mon, Jun 05, 2006 at 09:18:18PM -0400, Jeff Garzik wrote:
> On Mon, Jun 05, 2006 at 01:53:09PM -0700, Greg KH wrote:
> > Why not just use the proper pci interface? Why poke around in another
> > pci device to steal an irq, when that irq might not even be valid?
> > (irqs are not valid until pci_enable_device() is called on them...)
>
> Answered this question the last time you asked.
>
> Answer: this is an embedded platform that needs such poking. The
> wireless device is _another_ device.
Ugh, sorry, too many patches, too many different threads... You are
right...
greg k-h
^ permalink raw reply
* Re: [NET]: Add netif_tx_lock
From: Roland Dreier @ 2006-06-06 6:01 UTC (permalink / raw)
To: David Miller; +Cc: herbert, mchan, jgarzik, netdev
In-Reply-To: <20060605.221018.70218518.davem@davemloft.net>
David> As long as you never take priv->lock while ->xmit_lock is
David> held your patch should be OK.
Duh ... unfortunately priv->lock is taken from interrupt context so
that patch isn't safe. A correct fix would be the following, which
leads to a trivial conversion to using netif_tx_lock().
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 1dae4b2..7a8ca5d 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -821,8 +821,8 @@ void ipoib_mcast_restart_task(void *dev_
ipoib_mcast_stop_thread(dev, 0);
- spin_lock_irqsave(&dev->xmit_lock, flags);
- spin_lock(&priv->lock);
+ spin_lock_bh(&dev->xmit_lock);
+ spin_lock_irqsave(&priv->lock, flags);
/*
* Unfortunately, the networking core only gives us a list of all of
@@ -895,8 +895,8 @@ void ipoib_mcast_restart_task(void *dev_
}
}
- spin_unlock(&priv->lock);
- spin_unlock_irqrestore(&dev->xmit_lock, flags);
+ spin_unlock_irqrestore(&priv->lock, flags);
+ spin_unlock_bh(&dev->xmit_lock);
/* We have to cancel outside of the spinlock */
list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {
^ permalink raw reply related
* Re: [PATCH 1/1] LSM-IPsec SELinux Authorize (with minor fix)
From: David Miller @ 2006-06-06 5:49 UTC (permalink / raw)
To: jmorris
Cc: cxzhang, netdev, chrisw, herbert, sds, tjaeger, latten, sergeh,
gcwilson, czhang.us
In-Reply-To: <Pine.LNX.4.64.0606060133220.9787@d.namei>
From: James Morris <jmorris@namei.org>
Date: Tue, 6 Jun 2006 01:37:04 -0400 (EDT)
> On Tue, 6 Jun 2006, Catherine Zhang wrote:
>
> > Minor fix per James' comment.
>
> Can you also add a Signed-off-by line?
>
> I can't recall if you were the original author. If not, we also need a
> From line (per Documentation/SubmittingPatches).
I'll apply this to my net-2.6.18 tree once this is all sorted
out. Thanks everyone.
^ permalink raw reply
* Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver
From: David Brownell @ 2006-06-06 5:41 UTC (permalink / raw)
To: linux-usb-devel
Cc: Oliver Neukum, Daniel Drake, netdev, Ulrich Kunitz,
John W. Linville
In-Reply-To: <200606040025.32275.oliver@neukum.org>
On Saturday 03 June 2006 3:25 pm, Oliver Neukum wrote:
> Am Samstag, 3. Juni 2006 21:35 schrieb Daniel Drake:
> > Oliver Neukum wrote:
> > > +static int read_mac_addr(struct zd_chip *chip, u8 *mac_addr)
> > > +{
> > > + static const zd_addr_t addr[2] = { CR_MAC_ADDR_P1, CR_MAC_ADDR_P2 };
> > > + return _read_mac_addr(chip, mac_addr, (const zd_addr_t *)addr);
> > > +}
> > >
> > > Why on the stack?
> >
> > Technically it's not on the stack because it is static const (it goes in
> > rodata), but I don't think that this invalidates your point. What's the
> > alternative? kmalloc and kfree every time?
>
> In this case rodata will work. However, if you ever switch to direct DMA
> it will fail. I really did overlook the const keyword.
On some platforms rodata will work; but it's not guaranteed on any.
See Documentation/DMA-mapping.txt and read the little section at the
top explaining what types of memory can be DMA'd from ... neither stack,
nor BSS, nor data, nor rodata, nor text are OK.
- Dave
^ permalink raw reply
* Re: [PATCH 1/1] LSM-IPsec SELinux Authorize (with minor fix)
From: James Morris @ 2006-06-06 5:37 UTC (permalink / raw)
To: Catherine Zhang
Cc: netdev, davem, chrisw, herbert, sds, tjaeger, latten, sergeh,
gcwilson, czhang.us
In-Reply-To: <20060606043818.GA4125@jiayuguan.watson.ibm.com>
On Tue, 6 Jun 2006, Catherine Zhang wrote:
> Minor fix per James' comment.
Can you also add a Signed-off-by line?
I can't recall if you were the original author. If not, we also need a
>From line (per Documentation/SubmittingPatches).
Thanks,
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: Linux kernel and laws
From: Evgeniy Polyakov @ 2006-06-06 5:33 UTC (permalink / raw)
To: Adrian Bunk
Cc: Jeff Garzik, Arjan van de Ven, Christoph Hellwig, Andrew Morton,
linux-kernel, netdev, linville
In-Reply-To: <20060605140226.GR3955@stusta.de>
On Mon, Jun 05, 2006 at 04:02:26PM +0200, Adrian Bunk (bunk@stusta.de) wrote:
> > Far too many people have a careless "U.S.A. laws suck, merge it anyway"
> > attitude.
> If someone would state a submission to the kernel might have issues
> according to Chinese laws, or Iranian laws, or Russian laws, would this
> be enough for keeping code out of the kernel?
Btw, did kernel hackers consulted with Papua New Guinea or bloody
Russian laws? It is possible that they have a law which forbids to write
open source code. So we should stop Linux kernel development and completely
remove it's sources from the Internet ASAP.
P.S. It is explicitly permitted to make reverse engineering in Russia.
--
Evgeniy Polyakov
^ permalink raw reply
* Re: [PATCH 2.6.18 1/3] tg3: Remove unnecessary tx_lock
From: Michael Chan @ 2006-06-06 5:10 UTC (permalink / raw)
To: Herbert Xu; +Cc: davem, jgarzik, netdev
Herbert Xu wrote:
> The paths where full lock is preceded by netif_tx_disable are
> obviously
> safe (well, once you take off the LLTX flag anyway).
> However, there are
> paths that don't do netif_tx_disable (e.g., tg3_set_rx_mode), are they
> safe as well?
>
In places where we don't call tg3_netif_stop() before tg3_full_lock(),
we are typically doing one of the following:
- changing tg3_flags or tg3_flags2
- reprogramming MAC address
- reprogramming interrupt coalescing values
- reprogramming the PHY
- setting the rx mode
The TX code does not do any of these so it is safe.
tg3_set_rx_mode() is actually called with xmit_lock held through
dev->set_multicast_list(). Or it is called after tg3_netif_stop() in
the VLAN registration calls (in my patch #3).
^ permalink raw reply
* Re: [NET]: Add netif_tx_lock
From: David Miller @ 2006-06-06 5:10 UTC (permalink / raw)
To: rdreier; +Cc: herbert, mchan, jgarzik, netdev
In-Reply-To: <adahd2ynank.fsf@cisco.com>
From: Roland Dreier <rdreier@cisco.com>
Date: Mon, 05 Jun 2006 21:57:51 -0700
> Roland> You know, looking at the ipoib code, I can't even recreate
> Roland> why xmit_lock is taken in the set_multicast_list method
> Roland> anyway, or how it works at all -- it seems
> Roland> set_multicast_list will always be called with xmit_lock
> Roland> already held. What the heck is going on?
>
> Never mind -- I see that the set_multicast_list work needs to be
> deferred to process context, so ipoib_mcast_restart_task() doesn't run
> directly from the call to set_multicast_list.
>
> I guess the fix in the current kernel is just something like the
> below. And in the netif_tx_lock() patch, the local_irq_save() /
> local_irq_restore() calls can just be removed.
>
> Am I on the right track?
>
> Anyway I won't push the patch below since the bug is harmless right
> now and it can be fixed up as part of the netif_tx_lock() patch.
As long as you never take priv->lock while ->xmit_lock is held
your patch should be OK.
^ permalink raw reply
* Re: [NET]: Add netif_tx_lock
From: David Miller @ 2006-06-06 5:09 UTC (permalink / raw)
To: rdreier; +Cc: herbert, mchan, jgarzik, netdev
In-Reply-To: <adad5dmnacd.fsf@cisco.com>
From: Roland Dreier <rdreier@cisco.com>
Date: Mon, 05 Jun 2006 22:04:34 -0700
> Am I right in thinking that dev->xmit_lock still serializes mc_list
> operations, even for LLTX drivers?
Correct.
^ permalink raw reply
* Re: [NET]: Add netif_tx_lock
From: Roland Dreier @ 2006-06-06 5:04 UTC (permalink / raw)
To: David Miller; +Cc: herbert, mchan, jgarzik, netdev
In-Reply-To: <20060605.215821.69020274.davem@davemloft.net>
David> Isn't the IPOIB driver LLTX and therefore the upper layers
David> are not taking the xmit_lock?
Yeah, but I think that should be OK. If I'm remembering the intention
of the code correctly, the reason xmit_lock is being taken there is
just to protect dev->mc_list -- and this is needed because ipoib has
to defer handling set_multicast_list to process context. Am I right
in thinking that dev->xmit_lock still serializes mc_list operations,
even for LLTX drivers?
- R.
^ permalink raw reply
* Re: [NET]: Add netif_tx_lock
From: David Miller @ 2006-06-06 4:58 UTC (permalink / raw)
To: rdreier; +Cc: herbert, mchan, jgarzik, netdev
In-Reply-To: <adalksanb0c.fsf@cisco.com>
From: Roland Dreier <rdreier@cisco.com>
Date: Mon, 05 Jun 2006 21:50:11 -0700
> Roland> Sorry, I haven't followed this thread closely. Can you
> Roland> expand on what the bug in ipoib's multicast locking is?
>
> You know, looking at the ipoib code, I can't even recreate why
> xmit_lock is taken in the set_multicast_list method anyway, or how it
> works at all -- it seems set_multicast_list will always be called with
> xmit_lock already held. What the heck is going on?
Isn't the IPOIB driver LLTX and therefore the upper layers are
not taking the xmit_lock?
^ permalink raw reply
* Re: [NET]: Add netif_tx_lock
From: Roland Dreier @ 2006-06-06 4:57 UTC (permalink / raw)
To: David Miller; +Cc: herbert, mchan, jgarzik, netdev
In-Reply-To: <adalksanb0c.fsf@cisco.com>
Roland> You know, looking at the ipoib code, I can't even recreate
Roland> why xmit_lock is taken in the set_multicast_list method
Roland> anyway, or how it works at all -- it seems
Roland> set_multicast_list will always be called with xmit_lock
Roland> already held. What the heck is going on?
Never mind -- I see that the set_multicast_list work needs to be
deferred to process context, so ipoib_mcast_restart_task() doesn't run
directly from the call to set_multicast_list.
I guess the fix in the current kernel is just something like the
below. And in the netif_tx_lock() patch, the local_irq_save() /
local_irq_restore() calls can just be removed.
Am I on the right track?
Anyway I won't push the patch below since the bug is harmless right
now and it can be fixed up as part of the netif_tx_lock() patch.
Thanks,
Roland
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index ec41c8f..5f3eaf1 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -814,13 +814,12 @@ void ipoib_mcast_restart_task(void *dev_
struct dev_mc_list *mclist;
struct ipoib_mcast *mcast, *tmcast;
LIST_HEAD(remove_list);
- unsigned long flags;
ipoib_dbg_mcast(priv, "restarting multicast task\n");
ipoib_mcast_stop_thread(dev, 0);
- spin_lock_irqsave(&dev->xmit_lock, flags);
+ spin_lock_bh(&dev->xmit_lock);
spin_lock(&priv->lock);
/*
@@ -895,7 +894,7 @@ void ipoib_mcast_restart_task(void *dev_
}
spin_unlock(&priv->lock);
- spin_unlock_irqrestore(&dev->xmit_lock, flags);
+ spin_unlock_bh(&dev->xmit_lock);
/* We have to cancel outside of the spinlock */
list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {
^ permalink raw reply related
* Re: [NET]: Add netif_tx_lock
From: David Miller @ 2006-06-06 4:57 UTC (permalink / raw)
To: rdreier; +Cc: herbert, mchan, jgarzik, netdev
In-Reply-To: <adapshmnbam.fsf@cisco.com>
From: Roland Dreier <rdreier@cisco.com>
Date: Mon, 05 Jun 2006 21:44:01 -0700
> > IPOIB is going to BUG() with this change. Because now, in their
> > multicast code, you're going to local_bh_disable() via
> > netif_tx_unlock() with hw IRQs disabled which is illegal.
> >
> > It shows a bug here in the locking of the IPOIB driver.
>
> Sorry, I haven't followed this thread closely. Can you expand on what
> the bug in ipoib's multicast locking is?
It disables hw interrupts, and takes the xmit_lock.
xmit_lock is a BH only spinlock, you can't take it from hw
interrupts, or inside of a HW irq protected spinlock.
You can take it on the outside of a HW irq lock protected
area like this:
spin_lock_bh(&dev->xmit_lock);
spin_lock_irq(&whatever_lock);
spin_unlock_irq(&whatever_lock);
spin_unlock_bh(&dev->xmit_lock);
but not the other way around.
^ permalink raw reply
* Re: [PATCH 1/1] LSM-IPsec SELinux Authorize (with minor fix)
From: James Morris @ 2006-06-06 4:51 UTC (permalink / raw)
To: Catherine Zhang
Cc: netdev, davem, chrisw, herbert, sds, tjaeger, latten, sergeh,
gcwilson, czhang.us
In-Reply-To: <20060606043818.GA4125@jiayuguan.watson.ibm.com>
On Tue, 6 Jun 2006, Catherine Zhang wrote:
> Minor fix per James' comment.
Acked-by: James Morris <jmorris@namei.org>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [NET]: Add netif_tx_lock
From: Roland Dreier @ 2006-06-06 4:50 UTC (permalink / raw)
To: David Miller; +Cc: herbert, mchan, jgarzik, netdev
In-Reply-To: <adapshmnbam.fsf@cisco.com>
Roland> Sorry, I haven't followed this thread closely. Can you
Roland> expand on what the bug in ipoib's multicast locking is?
You know, looking at the ipoib code, I can't even recreate why
xmit_lock is taken in the set_multicast_list method anyway, or how it
works at all -- it seems set_multicast_list will always be called with
xmit_lock already held. What the heck is going on?
- R.
^ permalink raw reply
* Re: [NET]: Add netif_tx_lock
From: Roland Dreier @ 2006-06-06 4:44 UTC (permalink / raw)
To: David Miller; +Cc: herbert, mchan, jgarzik, netdev
In-Reply-To: <20060605.213250.85688925.davem@davemloft.net>
> IPOIB is going to BUG() with this change. Because now, in their
> multicast code, you're going to local_bh_disable() via
> netif_tx_unlock() with hw IRQs disabled which is illegal.
>
> It shows a bug here in the locking of the IPOIB driver.
Sorry, I haven't followed this thread closely. Can you expand on what
the bug in ipoib's multicast locking is?
Thanks,
Roland
^ permalink raw reply
* [PATCH 1/1] LSM-IPsec SELinux Authorize (with minor fix)
From: Catherine Zhang @ 2006-06-06 4:38 UTC (permalink / raw)
To: James Morris, netdev, davem, chrisw, herbert, sds, tjaeger
Cc: latten, sergeh, gcwilson, czhang.us
Hi,
Minor fix per James' comment.
thanks,
Catherine
--
This patch contains a fix for the previous patch that adds security
contexts to IPsec policies and security associations. In the previous
patch, no authorization (besides the check for write permissions to
SAD and SPD) is required to delete IPsec policies and security
assocations with security contexts. Thus a user authorized to change
SAD and SPD can bypass the IPsec policy authorization by simply
deleteing policies with security contexts. To fix this security hole,
an additional authorization check is added for removing security
policies and security associations with security contexts.
Note that if no security context is supplied on add or present on
policy to be deleted, the SELinux module allows the change
unconditionally. The hook is called on deletion when no context is
present, which we may want to change. At present, I left it up to the
module.
LSM changes:
The patch adds two new LSM hooks: xfrm_policy_delete and
xfrm_state_delete. The new hooks are necessary to authorize deletion
of IPsec policies that have security contexts. The existing hooks
xfrm_policy_free and xfrm_state_free lack the context to do the
authorization, so I decided to split authorization of deletion and
memory management of security data, as is typical in the LSM
interface.
Use:
The new delete hooks are checked when xfrm_policy or xfrm_state are
deleted by either the xfrm_user interface (xfrm_get_policy,
xfrm_del_sa) or the pfkey interface (pfkey_spddelete, pfkey_delete).
SELinux changes:
The new policy_delete and state_delete functions are added.
Signed-off-by: Trent Jaeger <tjaeger@cse.psu.edu>
---
include/linux/security.h | 40 ++++++++++++++++++++++++++++++++++------
net/key/af_key.c | 17 +++++++++++------
net/xfrm/xfrm_user.c | 19 ++++++++++++-------
security/dummy.c | 12 ++++++++++++
security/selinux/hooks.c | 2 ++
security/selinux/include/xfrm.h | 2 ++
security/selinux/xfrm.c | 39 +++++++++++++++++++++++++++++++++++----
7 files changed, 108 insertions(+), 23 deletions(-)
diff -puN include/linux/security.h~lsm-secpeer-fix include/linux/security.h
--- linux-2.6.17-rc4-mm3/include/linux/security.h~lsm-secpeer-fix 2006-05-31 00:01:35.000000000 -0400
+++ linux-2.6.17-rc4-mm3-cxzhang/include/linux/security.h 2006-05-31 00:46:13.000000000 -0400
@@ -805,31 +805,37 @@ struct swap_info_struct;
* used by the XFRM system.
* @sec_ctx contains the security context information being provided by
* the user-level policy update program (e.g., setkey).
- * Allocate a security structure to the xp->selector.security field.
+ * Allocate a security structure to the xp->security field.
* The security field is initialized to NULL when the xfrm_policy is
* allocated.
* Return 0 if operation was successful (memory to allocate, legal context)
* @xfrm_policy_clone_security:
* @old contains an existing xfrm_policy in the SPD.
* @new contains a new xfrm_policy being cloned from old.
- * Allocate a security structure to the new->selector.security field
- * that contains the information from the old->selector.security field.
+ * Allocate a security structure to the new->security field
+ * that contains the information from the old->security field.
* Return 0 if operation was successful (memory to allocate).
* @xfrm_policy_free_security:
* @xp contains the xfrm_policy
- * Deallocate xp->selector.security.
+ * Deallocate xp->security.
+ * @xfrm_policy_delete_security:
+ * @xp contains the xfrm_policy.
+ * Authorize deletion of xp->security.
* @xfrm_state_alloc_security:
* @x contains the xfrm_state being added to the Security Association
* Database by the XFRM system.
* @sec_ctx contains the security context information being provided by
* the user-level SA generation program (e.g., setkey or racoon).
- * Allocate a security structure to the x->sel.security field. The
+ * Allocate a security structure to the x->security field. The
* security field is initialized to NULL when the xfrm_state is
* allocated.
* Return 0 if operation was successful (memory to allocate, legal context).
* @xfrm_state_free_security:
* @x contains the xfrm_state.
- * Deallocate x>sel.security.
+ * Deallocate x->security.
+ * @xfrm_state_delete_security:
+ * @x contains the xfrm_state.
+ * Authorize deletion of x->security.
* @xfrm_policy_lookup:
* @xp contains the xfrm_policy for which the access control is being
* checked.
@@ -1298,8 +1304,10 @@ struct security_operations {
int (*xfrm_policy_alloc_security) (struct xfrm_policy *xp, struct xfrm_user_sec_ctx *sec_ctx);
int (*xfrm_policy_clone_security) (struct xfrm_policy *old, struct xfrm_policy *new);
void (*xfrm_policy_free_security) (struct xfrm_policy *xp);
+ int (*xfrm_policy_delete_security) (struct xfrm_policy *xp);
int (*xfrm_state_alloc_security) (struct xfrm_state *x, struct xfrm_user_sec_ctx *sec_ctx);
void (*xfrm_state_free_security) (struct xfrm_state *x);
+ int (*xfrm_state_delete_security) (struct xfrm_state *x);
int (*xfrm_policy_lookup)(struct xfrm_policy *xp, u32 sk_sid, u8 dir);
#endif /* CONFIG_SECURITY_NETWORK_XFRM */
@@ -2934,11 +2942,21 @@ static inline void security_xfrm_policy_
security_ops->xfrm_policy_free_security(xp);
}
+static inline int security_xfrm_policy_delete(struct xfrm_policy *xp)
+{
+ return security_ops->xfrm_policy_delete_security(xp);
+}
+
static inline int security_xfrm_state_alloc(struct xfrm_state *x, struct xfrm_user_sec_ctx *sec_ctx)
{
return security_ops->xfrm_state_alloc_security(x, sec_ctx);
}
+static inline int security_xfrm_state_delete(struct xfrm_state *x)
+{
+ return security_ops->xfrm_state_delete_security(x);
+}
+
static inline void security_xfrm_state_free(struct xfrm_state *x)
{
security_ops->xfrm_state_free_security(x);
@@ -2963,6 +2981,11 @@ static inline void security_xfrm_policy_
{
}
+static inline int security_xfrm_policy_delete(struct xfrm_policy *xp)
+{
+ return 0;
+}
+
static inline int security_xfrm_state_alloc(struct xfrm_state *x, struct xfrm_user_sec_ctx *sec_ctx)
{
return 0;
@@ -2972,6 +2995,11 @@ static inline void security_xfrm_state_f
{
}
+static inline int security_xfrm_state_delete(struct xfrm_policy *xp)
+{
+ return 0;
+}
+
static inline int security_xfrm_policy_lookup(struct xfrm_policy *xp, u32 sk_sid, u8 dir)
{
return 0;
diff -puN net/key/af_key.c~lsm-secpeer-fix net/key/af_key.c
--- linux-2.6.17-rc4-mm3/net/key/af_key.c~lsm-secpeer-fix 2006-05-31 00:01:46.000000000 -0400
+++ linux-2.6.17-rc4-mm3-cxzhang/net/key/af_key.c 2006-05-31 00:04:22.000000000 -0400
@@ -1454,21 +1454,23 @@ static int pfkey_delete(struct sock *sk,
if (x == NULL)
return -ESRCH;
+ if ((err = security_xfrm_state_delete(x)))
+ goto out;
+
if (xfrm_state_kern(x)) {
- xfrm_state_put(x);
- return -EPERM;
+ err = -EPERM;
+ goto out;
}
err = xfrm_state_delete(x);
- if (err < 0) {
- xfrm_state_put(x);
- return err;
- }
+ if (err < 0)
+ goto out;
c.seq = hdr->sadb_msg_seq;
c.pid = hdr->sadb_msg_pid;
c.event = XFRM_MSG_DELSA;
km_state_notify(x, &c);
+out:
xfrm_state_put(x);
return err;
@@ -2274,11 +2276,14 @@ static int pfkey_spddelete(struct sock *
err = 0;
+ if ((err = security_xfrm_policy_delete(xp)))
+ goto out;
c.seq = hdr->sadb_msg_seq;
c.pid = hdr->sadb_msg_pid;
c.event = XFRM_MSG_DELPOLICY;
km_policy_notify(xp, pol->sadb_x_policy_dir-1, &c);
+out:
xfrm_pol_put(xp);
return err;
}
diff -puN net/xfrm/xfrm_user.c~lsm-secpeer-fix net/xfrm/xfrm_user.c
--- linux-2.6.17-rc4-mm3/net/xfrm/xfrm_user.c~lsm-secpeer-fix 2006-05-31 00:01:54.000000000 -0400
+++ linux-2.6.17-rc4-mm3-cxzhang/net/xfrm/xfrm_user.c 2006-05-31 00:04:22.000000000 -0400
@@ -427,23 +427,25 @@ static int xfrm_del_sa(struct sk_buff *s
if (x == NULL)
return -ESRCH;
+ if (err = security_xfrm_state_delete(x))
+ goto out;
+
if (xfrm_state_kern(x)) {
- xfrm_state_put(x);
- return -EPERM;
+ err = -EPERM;
+ goto out;
}
err = xfrm_state_delete(x);
- if (err < 0) {
- xfrm_state_put(x);
- return err;
- }
+ if (err < 0)
+ goto out;
c.seq = nlh->nlmsg_seq;
c.pid = nlh->nlmsg_pid;
c.event = nlh->nlmsg_type;
km_state_notify(x, &c);
- xfrm_state_put(x);
+out:
+ xfrm_state_put(x);
return err;
}
@@ -1055,6 +1057,8 @@ static int xfrm_get_policy(struct sk_buf
MSG_DONTWAIT);
}
} else {
+ if (err = security_xfrm_policy_delete(xp))
+ goto out;
c.data.byid = p->index;
c.event = nlh->nlmsg_type;
c.seq = nlh->nlmsg_seq;
@@ -1064,6 +1068,7 @@ static int xfrm_get_policy(struct sk_buf
xfrm_pol_put(xp);
+out:
return err;
}
diff -puN security/dummy.c~lsm-secpeer-fix security/dummy.c
--- linux-2.6.17-rc4-mm3/security/dummy.c~lsm-secpeer-fix 2006-05-31 00:02:04.000000000 -0400
+++ linux-2.6.17-rc4-mm3-cxzhang/security/dummy.c 2006-05-31 00:04:22.000000000 -0400
@@ -810,6 +810,11 @@ static void dummy_xfrm_policy_free_secur
{
}
+static int dummy_xfrm_policy_delete_security(struct xfrm_policy *xp)
+{
+ return 0;
+}
+
static int dummy_xfrm_state_alloc_security(struct xfrm_state *x, struct xfrm_user_sec_ctx *sec_ctx)
{
return 0;
@@ -819,6 +824,11 @@ static void dummy_xfrm_state_free_securi
{
}
+static int dummy_xfrm_state_delete_security(struct xfrm_state *x)
+{
+ return 0;
+}
+
static int dummy_xfrm_policy_lookup(struct xfrm_policy *xp, u32 sk_sid, u8 dir)
{
return 0;
@@ -1024,8 +1034,10 @@ void security_fixup_ops (struct security
set_to_dummy_if_null(ops, xfrm_policy_alloc_security);
set_to_dummy_if_null(ops, xfrm_policy_clone_security);
set_to_dummy_if_null(ops, xfrm_policy_free_security);
+ set_to_dummy_if_null(ops, xfrm_policy_delete_security);
set_to_dummy_if_null(ops, xfrm_state_alloc_security);
set_to_dummy_if_null(ops, xfrm_state_free_security);
+ set_to_dummy_if_null(ops, xfrm_state_delete_security);
set_to_dummy_if_null(ops, xfrm_policy_lookup);
#endif /* CONFIG_SECURITY_NETWORK_XFRM */
#ifdef CONFIG_KEYS
diff -puN security/selinux/hooks.c~lsm-secpeer-fix security/selinux/hooks.c
--- linux-2.6.17-rc4-mm3/security/selinux/hooks.c~lsm-secpeer-fix 2006-05-31 00:02:11.000000000 -0400
+++ linux-2.6.17-rc4-mm3-cxzhang/security/selinux/hooks.c 2006-05-31 00:04:22.000000000 -0400
@@ -4400,8 +4400,10 @@ static struct security_operations selinu
.xfrm_policy_alloc_security = selinux_xfrm_policy_alloc,
.xfrm_policy_clone_security = selinux_xfrm_policy_clone,
.xfrm_policy_free_security = selinux_xfrm_policy_free,
+ .xfrm_policy_delete_security = selinux_xfrm_policy_delete,
.xfrm_state_alloc_security = selinux_xfrm_state_alloc,
.xfrm_state_free_security = selinux_xfrm_state_free,
+ .xfrm_state_delete_security = selinux_xfrm_state_delete,
.xfrm_policy_lookup = selinux_xfrm_policy_lookup,
#endif
};
diff -puN security/selinux/include/xfrm.h~lsm-secpeer-fix security/selinux/include/xfrm.h
--- linux-2.6.17-rc4-mm3/security/selinux/include/xfrm.h~lsm-secpeer-fix 2006-05-31 00:02:19.000000000 -0400
+++ linux-2.6.17-rc4-mm3-cxzhang/security/selinux/include/xfrm.h 2006-05-31 00:04:22.000000000 -0400
@@ -9,8 +9,10 @@
int selinux_xfrm_policy_alloc(struct xfrm_policy *xp, struct xfrm_user_sec_ctx *sec_ctx);
int selinux_xfrm_policy_clone(struct xfrm_policy *old, struct xfrm_policy *new);
void selinux_xfrm_policy_free(struct xfrm_policy *xp);
+int selinux_xfrm_policy_delete(struct xfrm_policy *xp);
int selinux_xfrm_state_alloc(struct xfrm_state *x, struct xfrm_user_sec_ctx *sec_ctx);
void selinux_xfrm_state_free(struct xfrm_state *x);
+int selinux_xfrm_state_delete(struct xfrm_state *x);
int selinux_xfrm_policy_lookup(struct xfrm_policy *xp, u32 sk_sid, u8 dir);
/*
diff -puN security/selinux/xfrm.c~lsm-secpeer-fix security/selinux/xfrm.c
--- linux-2.6.17-rc4-mm3/security/selinux/xfrm.c~lsm-secpeer-fix 2006-05-31 00:02:28.000000000 -0400
+++ linux-2.6.17-rc4-mm3-cxzhang/security/selinux/xfrm.c 2006-06-06 00:24:37.000000000 -0400
@@ -132,10 +132,7 @@ static int selinux_xfrm_sec_ctx_alloc(st
goto out;
/*
- * Does the subject have permission to set security or permission to
- * do the relabel?
- * Must be permitted to relabel from default socket type (process type)
- * to specified context
+ * Does the subject have permission to set security context?
*/
rc = avc_has_perm(tsec->sid, ctx->ctx_sid,
SECCLASS_ASSOCIATION,
@@ -201,6 +198,23 @@ void selinux_xfrm_policy_free(struct xfr
}
/*
+ * LSM hook implementation that authorizes deletion of labeled policies.
+ */
+int selinux_xfrm_policy_delete(struct xfrm_policy *xp)
+{
+ struct task_security_struct *tsec = current->security;
+ struct xfrm_sec_ctx *ctx = xp->security;
+ int rc = 0;
+
+ if (ctx)
+ rc = avc_has_perm(tsec->sid, ctx->ctx_sid,
+ SECCLASS_ASSOCIATION,
+ ASSOCIATION__SETCONTEXT, NULL);
+
+ return rc;
+}
+
+/*
* LSM hook implementation that allocs and transfers sec_ctx spec to
* xfrm_state.
*/
@@ -292,6 +306,23 @@ u32 selinux_socket_getpeer_dgram(struct
return SECSID_NULL;
}
+ /*
+ * LSM hook implementation that authorizes deletion of labeled SAs.
+ */
+int selinux_xfrm_state_delete(struct xfrm_state *x)
+{
+ struct task_security_struct *tsec = current->security;
+ struct xfrm_sec_ctx *ctx = x->security;
+ int rc = 0;
+
+ if (ctx)
+ rc = avc_has_perm(tsec->sid, ctx->ctx_sid,
+ SECCLASS_ASSOCIATION,
+ ASSOCIATION__SETCONTEXT, NULL);
+
+ return rc;
+}
+
/*
* LSM hook that controls access to unlabelled packets. If
* a xfrm_state is authorizable (defined by macro) then it was
_
^ permalink raw reply
* Re: [NET]: Add netif_tx_lock
From: David Miller @ 2006-06-06 4:32 UTC (permalink / raw)
To: herbert; +Cc: mchan, jgarzik, netdev
In-Reply-To: <20060601111503.GA16097@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 1 Jun 2006 21:15:04 +1000
> On Thu, Jun 01, 2006 at 10:25:25AM +1000, herbert wrote:
> >
> > > I think this netpoll wrinkle means we also have to make
> > > sure to set the xmit_lock_owner across the board.
> >
> > You're right. In fact this can deadlock today for those drivers that
> > already make use of xmit_lock without setting the owner. So I suppose
> > something like net_xmit_lock to obtain xmit_lock is called for.
>
> OK, here is a patch which does this.
>
> [NET]: Add netif_tx_lock
IPOIB is going to BUG() with this change. Because now, in their
multicast code, you're going to local_bh_disable() via
netif_tx_unlock() with hw IRQs disabled which is illegal.
It shows a bug here in the locking of the IPOIB driver.
We need to think about this change some more. :)
^ permalink raw reply
* Re: [PATCH] unsigned long => __be32 in icmp_redirect()
From: David Miller @ 2006-06-06 4:18 UTC (permalink / raw)
To: adobriyan; +Cc: netdev
In-Reply-To: <20060527212925.GC7267@martell.zuzino.mipt.ru>
From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Sun, 28 May 2006 01:29:25 +0400
> ip_rt_redirect() expects __be32 as second arg, so...
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
I'm going to cure this by simple killing the 'ip' variable
altogether. It is assigned once, to iph->daddr, and used
once, as an argument. So just pass iph->daddr directly.
Thanks for finding this.
^ permalink raw reply
* Re: [PATCH] Right prototype of __raw_v4_lookup()
From: David Miller @ 2006-06-06 4:07 UTC (permalink / raw)
To: adobriyan; +Cc: netdev
In-Reply-To: <20060527210932.GB7267@martell.zuzino.mipt.ru>
From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Sun, 28 May 2006 01:09:32 +0400
> All users pass 32-bit values as addresses and internally they're compared with
> 32-bit entities. So, change "laddr" and "raddr" types to __be32.
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
Applied.
Doesn't this mean that, at least eventually, we should change
the address members of inet_sock to __be32 as well?
^ permalink raw reply
* Re: [PATCH] Fixup struct ip_mc_list::multiaddr type
From: David Miller @ 2006-06-06 4:04 UTC (permalink / raw)
To: adobriyan; +Cc: netdev
In-Reply-To: <20060526193009.GB7266@martell.zuzino.mipt.ru>
From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Fri, 26 May 2006 23:30:09 +0400
> All users except two expect 32-bit big-endian value. One is of
>
> ->multiaddr = ->multiaddr
>
> variety. And last one is "%08lX".
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
Applied, thanks Alexey.
^ 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