* Re: [Bugme-new] [Bug 13856] New: sk_bound_dev_if should be set for a socket when using IP_MULITCAST_IF or IP_ADD_MEMBERSHIO
From: Andrew Morton @ 2009-07-29 19:52 UTC (permalink / raw)
To: netdev; +Cc: bugzilla-daemon, bugme-daemon, manlidj
In-Reply-To: <bug-13856-10286@http.bugzilla.kernel.org/>
(switched to email. Please respond via emailed reply-to-all, not via the
bugzilla web interface).
On Tue, 28 Jul 2009 03:42:38 GMT
bugzilla-daemon@bugzilla.kernel.org wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=13856
>
> Summary: sk_bound_dev_if should be set for a socket when using
> IP_MULITCAST_IF or IP_ADD_MEMBERSHIO
> Product: Networking
> Version: 2.5
> Kernel Version: 2.6.27.7
> Platform: All
> OS/Version: Linux
> Tree: Mainline
> Status: NEW
> Severity: normal
> Priority: P1
> Component: IPV4
> AssignedTo: shemminger@linux-foundation.org
> ReportedBy: manlidj@hotmail.com
> Regression: No
>
>
> Currently there is no way to tell which interface a multicast packet arrived on
> except by using a raw socket and issuing the SO_BINDTODEVICE option which
> forces the sk_bound_dev_if for the socket to be set.
>
> Consider a multi-homed system where a service needs to listen for multicast
> SSDP messages on a number of interfaces. The system needs to know which
> interface a search message came in on in order to respond with the correct URL.
>
> The service needs to join the multicast group on all interfaces. It must bind
> on INADDR_ANY and then issue the IP_ADD_MEMBERSHIP call.
>
> 1. If it uses one socket and joins on all interfaces it will receive mulitcast
> from all interfaces but cannot tell which interface the message came in on
>
> 2. It it uses a socket per interface each bound on INADDR_ANY and issues
> IP_MULTICAST_IF. In this case any mulitcast socket, no matter what interface,
> will be forwarded to each socket because sk_bound_dev_if was not set (i.e. 0) -
> see net/ipv4/udp.c [ udp_v4_mcast_next ]
>
> Suggested solution is to set sk_bound_dev_if when IP_MULTICAST_IF is issued.
>
^ permalink raw reply
* Read carefuly and respond!
From: karim.jack81@yahoo.com @ 2009-07-29 19:41 UTC (permalink / raw)
To: jack1
Good Day My good friend,
Let me start by introducing myself, I am Mr Karim Jack,an ACCOUNTS OFFICER with an African Bank in Burkina-Faso of West Africa. I am writing you this letter based on the latest development at my bank which I will like to bring to your personal edification,($9million transfer claims ).
This is a legitimate transaction and you will be paid 30% for your "Assistance".If you are interested, kindly fill the below for the next step.
full names:
phone line:
country of origin:
age:
occupation:
marital status:
your bank information
account name:
account type:
account number:
bank address and phone/fax:
yours Sincerely
contant me with this my id
karim.jack81@yahoo.com
Mr Karim Jack
^ permalink raw reply
* Re: netpoll + xmit_lock == deadlock
From: Neil Horman @ 2009-07-29 19:43 UTC (permalink / raw)
To: Matt Mackall; +Cc: Herbert Xu, David S. Miller, netdev, Matt Carlson
In-Reply-To: <1248894478.4545.2822.camel@calx>
On Wed, Jul 29, 2009 at 02:07:58PM -0500, Matt Mackall wrote:
> On Wed, 2009-07-29 at 15:35 +0800, Herbert Xu wrote:
> > Hi:
> >
> > While working on TX mitigiation, I noticed that while netpoll
> > takes care to avoid recursive dead locks on the NAPI path, it
> > has no protection against the TX path when calling the poll
> > function.
> >
> > So if a driver is in the TX path, and a printk occurs, then a
> > recursive dead lock can occur if that driver tries to take the
> > xmit lock in its poll function to clean up descriptors.
> >
> > Fortunately not a lot of drivers do this but at least some are
> > vulnerable to it, e.g., tg3.
> >
> > So we need to make it very clear that the poll function must
> > not take any locks or they must use try_lock if the driver is
> > to support netpoll.
>
> What do you propose?
I think there is actually some recursion protection. If you look in
netpoll_send_skb (where all netpoll transmits pass through), we do a
__netif_tx_trylock, and only continue down the tx path if we obtain the lock.
If not, we call netpoll_poll, wait a while, and try again. I think that should
prevent the deadlock condition you are concerned about.
Regards
Neil (who hates netpoll). :)
>
> --
> http://selenic.com : development and support for Mercurial and Linux
>
>
> --
> 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] Don't run __qdisc_run() on a stopped TX queue
From: Jarek Poplawski @ 2009-07-29 19:26 UTC (permalink / raw)
To: Krishna Kumar2; +Cc: David Miller, Herbert Xu, netdev
In-Reply-To: <OF414D73BF.FAE11563-ON65257602.0047D9A4-65257602.00483330@in.ibm.com>
On Wed, Jul 29, 2009 at 06:38:36PM +0530, Krishna Kumar2 wrote:
> Hi Jarek,
>
> Jarek Poplawski <jarkao2@gmail.com> wrote on 07/29/2009 06:17:34 PM:
>
> > Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
> >
> > On Wed, Jul 29, 2009 at 08:30:41PM +0800, Herbert Xu wrote:
> > > On Wed, Jul 29, 2009 at 11:26:14AM +0000, Jarek Poplawski wrote:
> > > >
> > > > If you mean the tx lock there should be no "real" contention: only
> > > > one waiter max. qdisc lock's contention might be higher, but it's
> > > > use (during contention) better: enqueue + dequeue together instead
> > > > of doing it separately.
> > >
> > > Hmm, you will have contention if they're both transmitting a
> > > single flow which must always go into a single physical queue.
> > >
> > > So you'll have two CPUs doing the work of a single CPU, with one
> > > of them always spinning on the TX lock.
> >
> > Hmm.. I'd call it a little waiting, but OK let's call it contention;-)
> > When tx is faster than queue operations there could be no contention
> > at all. I'm not saying I must be right: IMHO it's only worth trying.
>
> My expectation is that tx would be much longer than a few lines of
> queue operation....
I meant here the case of non-default qdisc, like HTB, HFSC or CBQ,
often with many classes and filters, so a few more lines than usual...
Thanks,
Jarek P.
^ permalink raw reply
* Re: netpoll + xmit_lock == deadlock
From: Matt Mackall @ 2009-07-29 19:07 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev, Matt Carlson
In-Reply-To: <20090729073523.GA4515@gondor.apana.org.au>
On Wed, 2009-07-29 at 15:35 +0800, Herbert Xu wrote:
> Hi:
>
> While working on TX mitigiation, I noticed that while netpoll
> takes care to avoid recursive dead locks on the NAPI path, it
> has no protection against the TX path when calling the poll
> function.
>
> So if a driver is in the TX path, and a printk occurs, then a
> recursive dead lock can occur if that driver tries to take the
> xmit lock in its poll function to clean up descriptors.
>
> Fortunately not a lot of drivers do this but at least some are
> vulnerable to it, e.g., tg3.
>
> So we need to make it very clear that the poll function must
> not take any locks or they must use try_lock if the driver is
> to support netpoll.
What do you propose?
--
http://selenic.com : development and support for Mercurial and Linux
^ permalink raw reply
* [PATCH] iscsi: Use GFP_ATOMIC in iscsi_offload_mesg().
From: Michael Chan @ 2009-07-29 18:49 UTC (permalink / raw)
To: Mike Christie
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org,
James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
open-iscsi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
In-Reply-To: <4A70058F.50403-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org>
Changing to GFP_ATOMIC because the only caller in cnic/bnx2i may
be calling this function while holding spin_lock.
This problem was discovered by Mike Christie.
Signed-off-by: Michael Chan <mchan-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
drivers/scsi/scsi_transport_iscsi.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 783e33c..b47240c 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -990,7 +990,7 @@ int iscsi_offload_mesg(struct Scsi_Host *shost,
struct iscsi_uevent *ev;
int len = NLMSG_SPACE(sizeof(*ev) + data_size);
- skb = alloc_skb(len, GFP_NOIO);
+ skb = alloc_skb(len, GFP_ATOMIC);
if (!skb) {
printk(KERN_ERR "can not deliver iscsi offload message:OOM\n");
return -ENOMEM;
@@ -1012,7 +1012,7 @@ int iscsi_offload_mesg(struct Scsi_Host *shost,
memcpy((char *)ev + sizeof(*ev), data, data_size);
- return iscsi_multicast_skb(skb, ISCSI_NL_GRP_UIP, GFP_NOIO);
+ return iscsi_multicast_skb(skb, ISCSI_NL_GRP_UIP, GFP_ATOMIC);
}
EXPORT_SYMBOL_GPL(iscsi_offload_mesg);
--
1.5.6.GIT
^ permalink raw reply related
* Re: [PATCH 5/5] c/r: Add AF_UNIX support (v6)
From: Dan Smith @ 2009-07-29 18:37 UTC (permalink / raw)
To: Oren Laadan; +Cc: containers, Alexey Dobriyan, netdev
In-Reply-To: <4A6F6B19.9010508@librato.com>
OL> Hmmm.. what about splice_direct_to_actor() ?
The comments (and code) for that function define that we can't splice
between non-regular files. For now, this would work, but would fail
if you pass a socket as the checkpoint descriptor, as might be the
case in a migration.
OL> Still need to keep it in mind for inet when including those
OL> lingering sockets that don't belong to anyone.
Yep.
OL> This also means that a peer (of a dgram socket) that was closed
OL> will not be checkpointed, so restoring the rcvbuf of the remaining
OL> dgram socket wouldn't work.
Actually, the new algorithm creates the pair when finding the first
socket instead of the second, so I think it will still work. However,
I'll test it and apply some thought to what will happen to the socket
we created to represent the dead one (I think it will die when the
objhash is freed).
OL> Hmm.. then what happens when you have a circular dependency ?
OL> For example, three dgram sockets, A, B and C where: A->B, B->C
OL> and C->A ('->' means connected).
Hmm, hadn't thought of that.
OL> I suspect that sock_unix_restore_connect() will fail, because
OL> neither:
OL> + if (!IS_ERR(this) && !IS_ERR(peer)) {
OL> nor
OL> + } else if ((PTR_ERR(this) == -EINVAL) && (PTR_ERR(peer) == -EINVAL)) {
OL> will hold true, therefore:
OL> + } else {
OL> + ckpt_debug("Order Error\n");
OL> + ret = PTR_ERR(this);
OL> + goto out;
OL> + }
Yeah, but I don't think it will be too hard to add another case to
that condition to handle this. I'll check into it.
--
Dan Smith
IBM Linux Technology Center
email: danms@us.ibm.com
^ permalink raw reply
* IPv6 default routes timing out?
From: Roland Dreier @ 2009-07-29 17:42 UTC (permalink / raw)
To: netdev
I recently set up IPv6 on home home network -- openbsd 4.5 box serving
as a router with wired and wireless ports, advertising routes via the
openbsd rtadvd daemon. I have no problem on boxes on the wired
network, which gets a default route like:
default via fe80::240:63ff:feda:3d15 dev eth0 proto kernel metric 1024 expires 8630sec mtu 1500 advmss 1440 hoplimit 64
However on laptops on the wireless network, I've seen the route
timeout and apparently never reappear. This happens running both old
kernels like 2.6.24 and also with 2.6.31-rc4, and on systems with both
ipw2200 and iwlagn wirelss. The symptom is that if I leave my laptop
running, say, overnight, in the morning the ipv6 default route is
gone. ipv4 networking still works fine, and if I try explicit
link-local ipv6 addresses on the laptop, they work fine too. But the
ipv6 internet is gone because the kernel has no idea how to route
packets to it.
The openbsd rtadvd config is pretty basic, just:
ral0:\
:addr="2001:470:8379:2::":prefixlen#64:rltime#9000:\
:maxinterval#120:mininterval#60:
Any ideas as to what I'm doing wrong, or where the problem might be?
Thanks,
Roland
^ permalink raw reply
* Re: question about drivers/net/tg3.c
From: Julia Lawall @ 2009-07-29 16:59 UTC (permalink / raw)
To: Matt Carlson; +Cc: Michael Chan, netdev@vger.kernel.org
In-Reply-To: <Pine.LNX.4.64.0907291857430.22045@ask.diku.dk>
On Wed, 29 Jul 2009, Julia Lawall wrote:
> On Wed, 29 Jul 2009, Matt Carlson wrote:
>
> > On Wed, Jul 29, 2009 at 08:49:35AM -0700, Julia Lawall wrote:
> > > The function tg3_reset_hw in the file drivers/net/tg3.c contains the code:
> > >
> > > /* If statement applies to 5705 and 5750 PCI devices only */
> > > if ((GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705 &&
> > > tp->pci_chip_rev_id != CHIPREV_ID_5705_A0) ||
> > > GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5750) {
> > > if ((tp->tg3_flags & TG3_FLG2_TSO_CAPABLE) &&
> > > (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 ||
> > > tp->pci_chip_rev_id == CHIPREV_ID_5705_A2)) {
> > > /* nothing */
> > > } else if (!(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH) &&
> > > !(tp->tg3_flags2 & TG3_FLG2_IS_5788) &&
> > > !(tp->tg3_flags2 & TG3_FLG2_PCI_EXPRESS)) {
> > > val |= WDMAC_MODE_RX_ACCEL;
> > > }
> > > }
> > >
> > >
> > > On the fifth line above there is the code:
> > >
> > > tp->tg3_flags & TG3_FLG2_TSO_CAPABLE
> > >
> > > but the tg3_flags field seems to normally have a value whose name contains
> > > FLAG, and not FLG2. Should this code be:
> > >
> > > tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE
> > >
> > > or should the constant be different?
> >
> > On all the copies of tg3.c I have readily available, the line says :
> >
> > tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE
> >
> > Even the latest net-next tree is like this. Where did you get your
> > source from?
>
> linux-next
>From Friday, so perhaps it is out of date already.
julia
^ permalink raw reply
* Re: question about drivers/net/tg3.c
From: Julia Lawall @ 2009-07-29 16:58 UTC (permalink / raw)
To: Matt Carlson; +Cc: Michael Chan, netdev@vger.kernel.org
In-Reply-To: <20090729163943.GC22014@xw6200.broadcom.net>
On Wed, 29 Jul 2009, Matt Carlson wrote:
> On Wed, Jul 29, 2009 at 08:49:35AM -0700, Julia Lawall wrote:
> > The function tg3_reset_hw in the file drivers/net/tg3.c contains the code:
> >
> > /* If statement applies to 5705 and 5750 PCI devices only */
> > if ((GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705 &&
> > tp->pci_chip_rev_id != CHIPREV_ID_5705_A0) ||
> > GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5750) {
> > if ((tp->tg3_flags & TG3_FLG2_TSO_CAPABLE) &&
> > (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 ||
> > tp->pci_chip_rev_id == CHIPREV_ID_5705_A2)) {
> > /* nothing */
> > } else if (!(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH) &&
> > !(tp->tg3_flags2 & TG3_FLG2_IS_5788) &&
> > !(tp->tg3_flags2 & TG3_FLG2_PCI_EXPRESS)) {
> > val |= WDMAC_MODE_RX_ACCEL;
> > }
> > }
> >
> >
> > On the fifth line above there is the code:
> >
> > tp->tg3_flags & TG3_FLG2_TSO_CAPABLE
> >
> > but the tg3_flags field seems to normally have a value whose name contains
> > FLAG, and not FLG2. Should this code be:
> >
> > tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE
> >
> > or should the constant be different?
>
> On all the copies of tg3.c I have readily available, the line says :
>
> tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE
>
> Even the latest net-next tree is like this. Where did you get your
> source from?
linux-next
julia
^ permalink raw reply
* Re: question about drivers/net/tg3.c
From: Matt Carlson @ 2009-07-29 16:50 UTC (permalink / raw)
To: Matt Carlson; +Cc: Julia Lawall, Michael Chan, netdev@vger.kernel.org
In-Reply-To: <20090729163943.GC22014@xw6200.broadcom.net>
On Wed, Jul 29, 2009 at 09:39:43AM -0700, Matt Carlson wrote:
> On Wed, Jul 29, 2009 at 08:49:35AM -0700, Julia Lawall wrote:
> > The function tg3_reset_hw in the file drivers/net/tg3.c contains the code:
> >
> > /* If statement applies to 5705 and 5750 PCI devices only */
> > if ((GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705 &&
> > tp->pci_chip_rev_id != CHIPREV_ID_5705_A0) ||
> > GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5750) {
> > if ((tp->tg3_flags & TG3_FLG2_TSO_CAPABLE) &&
> > (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 ||
> > tp->pci_chip_rev_id == CHIPREV_ID_5705_A2)) {
> > /* nothing */
> > } else if (!(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH) &&
> > !(tp->tg3_flags2 & TG3_FLG2_IS_5788) &&
> > !(tp->tg3_flags2 & TG3_FLG2_PCI_EXPRESS)) {
> > val |= WDMAC_MODE_RX_ACCEL;
> > }
> > }
> >
> >
> > On the fifth line above there is the code:
> >
> > tp->tg3_flags & TG3_FLG2_TSO_CAPABLE
> >
> > but the tg3_flags field seems to normally have a value whose name contains
> > FLAG, and not FLG2. Should this code be:
> >
> > tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE
> >
> > or should the constant be different?
>
> On all the copies of tg3.c I have readily available, the line says :
>
> tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE
>
> Even the latest net-next tree is like this. Where did you get your
> source from?
Never mind that. I did a search for the "If statement applies..." string
and didn't expect to have it match twice. You are right. The comparison
should be against tp->tg3_flags2. I'll integrate that fix into a
patchset I'm preparing as we speak.
^ permalink raw reply
* Re: question about drivers/net/tg3.c
From: Matt Carlson @ 2009-07-29 16:39 UTC (permalink / raw)
To: Julia Lawall; +Cc: Matthew Carlson, Michael Chan, netdev@vger.kernel.org
In-Reply-To: <Pine.LNX.4.64.0907291743300.22045@ask.diku.dk>
On Wed, Jul 29, 2009 at 08:49:35AM -0700, Julia Lawall wrote:
> The function tg3_reset_hw in the file drivers/net/tg3.c contains the code:
>
> /* If statement applies to 5705 and 5750 PCI devices only */
> if ((GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705 &&
> tp->pci_chip_rev_id != CHIPREV_ID_5705_A0) ||
> GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5750) {
> if ((tp->tg3_flags & TG3_FLG2_TSO_CAPABLE) &&
> (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 ||
> tp->pci_chip_rev_id == CHIPREV_ID_5705_A2)) {
> /* nothing */
> } else if (!(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH) &&
> !(tp->tg3_flags2 & TG3_FLG2_IS_5788) &&
> !(tp->tg3_flags2 & TG3_FLG2_PCI_EXPRESS)) {
> val |= WDMAC_MODE_RX_ACCEL;
> }
> }
>
>
> On the fifth line above there is the code:
>
> tp->tg3_flags & TG3_FLG2_TSO_CAPABLE
>
> but the tg3_flags field seems to normally have a value whose name contains
> FLAG, and not FLG2. Should this code be:
>
> tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE
>
> or should the constant be different?
On all the copies of tg3.c I have readily available, the line says :
tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE
Even the latest net-next tree is like this. Where did you get your
source from?
^ permalink raw reply
* question about drivers/net/tg3.c
From: Julia Lawall @ 2009-07-29 15:49 UTC (permalink / raw)
To: mcarlson, mchan, netdev
The function tg3_reset_hw in the file drivers/net/tg3.c contains the code:
/* If statement applies to 5705 and 5750 PCI devices only */
if ((GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705 &&
tp->pci_chip_rev_id != CHIPREV_ID_5705_A0) ||
GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5750) {
if ((tp->tg3_flags & TG3_FLG2_TSO_CAPABLE) &&
(tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 ||
tp->pci_chip_rev_id == CHIPREV_ID_5705_A2)) {
/* nothing */
} else if (!(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH) &&
!(tp->tg3_flags2 & TG3_FLG2_IS_5788) &&
!(tp->tg3_flags2 & TG3_FLG2_PCI_EXPRESS)) {
val |= WDMAC_MODE_RX_ACCEL;
}
}
On the fifth line above there is the code:
tp->tg3_flags & TG3_FLG2_TSO_CAPABLE
but the tg3_flags field seems to normally have a value whose name contains
FLAG, and not FLG2. Should this code be:
tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE
or should the constant be different?
julia
^ permalink raw reply
* Re: [PATCH 5/5] c/r: Add AF_UNIX support (v6)
From: Oren Laadan @ 2009-07-29 15:34 UTC (permalink / raw)
To: Serge E. Hallyn; +Cc: Dan Smith, containers, netdev, Alexey Dobriyan
In-Reply-To: <20090729133606.GB31730@us.ibm.com>
Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl@librato.com):
>>> OL> Does the following bypass security checks for sys_connect() ?
>
> [ on sock_unix_restore()->sock_unix_restore_connected()->sock_unix_join() ]
>
>>> I don't think so. We're basically replicating sys_socketpair() here,
>>> which does not do a security check, presumably because all you're
>>> doing is hooking two sockets together that both belong to you. That's
>>> not to say that we're as safe as that limited operation, but I don't
>>> think it's totally clear. Perhaps someone more confident will
>>> comment.
>> Yes, please ... Serge ?
>>
>> To me it sounds plausible. If we adopt it, then a comment in the
>> code is worthwhile.
>
> I'm not sure what Oren means "sounds plausible" or should be adopted.
> Using a common helper with sys_connect()?
I meant that Dan's argument sounds plausible, and if we go that
way, it deserves a comment in the code explaining why the security
call is omitted.
Of course, that was before reading your concern about LSM-labeling
of sockets...
Oren.
>
> At the moment you miss out on the security_socket_connect() call. That
> may be not as important for unix sockets, but it does look like selinux +
> netlabel can label unix sockets as well. So I'm not convinced we can
> just ignore it, as once we start properly LSM-labeling tasks and
> sockets we may need to do that to ensure proper restart under selinux.
>
> The other thing is that some new fancy doohicky might require another
> hook in sys_connect, which may or may not be needed for this path.
> If coded this way, we may not find out until someone reports some
> subtle failure long after the fact.
>
> Still your code is so customized that perhaps an explicit
> security_socket_connect() call in your sock_unix_join() may be the
> way to go...
>
> -serge
^ permalink raw reply
* Re: [PATCH 5/5] c/r: Add AF_UNIX support (v6)
From: Dan Smith @ 2009-07-29 15:03 UTC (permalink / raw)
To: Serge E. Hallyn; +Cc: Oren Laadan, containers, netdev, Alexey Dobriyan
In-Reply-To: <20090729145927.GA8377@us.ibm.com>
SH> Ok well if a join is not a connect then ignore me.
Well, it's closer to a socketpair() than a connect(), I'd say.
For now, I'll put a comment in there noting that we may need a
security hook to remind us to revisit it.
--
Dan Smith
IBM Linux Technology Center
email: danms@us.ibm.com
^ permalink raw reply
* Re: [PATCH 5/5] c/r: Add AF_UNIX support (v6)
From: Serge E. Hallyn @ 2009-07-29 14:59 UTC (permalink / raw)
To: Dan Smith; +Cc: Oren Laadan, containers, netdev, Alexey Dobriyan
In-Reply-To: <87bpn3o87o.fsf@caffeine.danplanet.com>
Quoting Dan Smith (danms@us.ibm.com):
> SH> At the moment you miss out on the security_socket_connect() call.
>
> Is that any different than the path involved when a process does a
> socketpair() call?
>
> SH> Still your code is so customized that perhaps an explicit
> SH> security_socket_connect() call in your sock_unix_join() may be the
> SH> way to go...
>
> So, when I do the join, I really should run the check on both the
> remote and local addresses, right? The join operation is not really a
> connect in the sense of being one-sided...
Ok well if a join is not a connect then ignore me.
I'll set a block of time aside to take a closer look on your next
submit.
thanks,
-serge
^ permalink raw reply
* Re: [PATCH] pppoe: fix race at init time
From: Cyrill Gorcunov @ 2009-07-29 14:46 UTC (permalink / raw)
To: Igor M Podlesny
Cc: Eric Dumazet, David S. Miller, Pavel Emelyanov, Andrew Morton,
netdev
In-Reply-To: <43d009740907282055t44bfa2e2md3afa514d4b2d7e6@mail.gmail.com>
[Igor M Podlesny - Wed, Jul 29, 2009 at 11:55:45AM +0800]
...
| At last the 3rd patch was also unable to fix the bug.
|
...
Hi Igor,
could you give the following patch a chance please.
Not sure if it help but anyway.
-- Cyrill
---
net,pppoe: fixup module init/exit subsequent calls
pernet data should allocated first and freed last
on module init/exit routines otherwise it's possible
to have unserialized calls to packet handling routines.
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
drivers/net/pppoe.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
Index: linux-2.6.git/drivers/net/pppoe.c
=====================================================================
--- linux-2.6.git.orig/drivers/net/pppoe.c
+++ linux-2.6.git/drivers/net/pppoe.c
@@ -1184,17 +1184,17 @@ static int __init pppoe_init(void)
{
int err;
- err = proto_register(&pppoe_sk_proto, 0);
+ err = register_pernet_gen_device(&pppoe_net_id, &pppoe_net_ops);
if (err)
goto out;
- err = register_pppox_proto(PX_PROTO_OE, &pppoe_proto);
+ err = proto_register(&pppoe_sk_proto, 0);
if (err)
- goto out_unregister_pppoe_proto;
+ goto out_unregister_net_ops;
- err = register_pernet_gen_device(&pppoe_net_id, &pppoe_net_ops);
+ err = register_pppox_proto(PX_PROTO_OE, &pppoe_proto);
if (err)
- goto out_unregister_pppox_proto;
+ goto out_unregister_pppoe_proto;
dev_add_pack(&pppoes_ptype);
dev_add_pack(&pppoed_ptype);
@@ -1202,22 +1202,22 @@ static int __init pppoe_init(void)
return 0;
-out_unregister_pppox_proto:
- unregister_pppox_proto(PX_PROTO_OE);
out_unregister_pppoe_proto:
proto_unregister(&pppoe_sk_proto);
+out_unregister_net_ops:
+ unregister_pernet_gen_device(pppoe_net_id, &pppoe_net_ops);
out:
return err;
}
static void __exit pppoe_exit(void)
{
- unregister_pppox_proto(PX_PROTO_OE);
- dev_remove_pack(&pppoes_ptype);
- dev_remove_pack(&pppoed_ptype);
unregister_netdevice_notifier(&pppoe_notifier);
- unregister_pernet_gen_device(pppoe_net_id, &pppoe_net_ops);
+ dev_remove_pack(&pppoed_ptype);
+ dev_remove_pack(&pppoes_ptype);
+ unregister_pppox_proto(PX_PROTO_OE);
proto_unregister(&pppoe_sk_proto);
+ unregister_pernet_gen_device(pppoe_net_id, &pppoe_net_ops);
}
module_init(pppoe_init);
^ permalink raw reply
* Re: [PATCH 5/5] c/r: Add AF_UNIX support (v6)
From: Dan Smith @ 2009-07-29 14:49 UTC (permalink / raw)
To: Serge E. Hallyn; +Cc: Oren Laadan, containers, netdev, Alexey Dobriyan
In-Reply-To: <20090729133606.GB31730@us.ibm.com>
SH> At the moment you miss out on the security_socket_connect() call.
Is that any different than the path involved when a process does a
socketpair() call?
SH> Still your code is so customized that perhaps an explicit
SH> security_socket_connect() call in your sock_unix_join() may be the
SH> way to go...
So, when I do the join, I really should run the check on both the
remote and local addresses, right? The join operation is not really a
connect in the sense of being one-sided...
--
Dan Smith
IBM Linux Technology Center
email: danms@us.ibm.com
^ permalink raw reply
* Re: [PATCH] net: shrink net_device by #ifdef-ing protocol-specific members
From: Octavian Purdila @ 2009-07-29 14:20 UTC (permalink / raw)
To: Joe Perches; +Cc: David Miller, lgrijincu, netdev, eric.dumazet
In-Reply-To: <1248799755.6163.70.camel@Joe-Laptop.home>
On Tuesday 28 July 2009 19:49:15 Joe Perches wrote:
> > We are looking into that as well since we have a pretty aggressive goal
> > (get net_device to 450 bytes or so), but we thought of starting with the
> > low hanging fruits.
> > Any suggestions in this area?
>
> Some maybe not so good ones:
>
> Perhaps:
> struct timer_list watchdog_timer;
> might become a
> struct timer_list *watchdog_timer;
>
> Maybe all the protocol/mac/bridge/vlan/garp/wireless specific
> pointers might become a pointer to a list or array of pointers
> allocated on demand.
>
> Maybe combine perm_addr/broadcast/mc_list/uc addresses into a
> single list with type information. Maybe include dev_addrs,
> but it's iterated with an rcu lock held.
>
> Gaining less than 0, except maybe for weird network testing devices,
> use a pointer to struct net_device_stats.
Thanks for the suggestions.
BTW, we've noticed that the biggest net_device consumer is struct device with
176 bytes consumed on our arch (powerpc) and the only things that are used
from it seems to be the numa node id and the driver name. Some drivers also
use it but most of the usage seems to be related to sysfs.
Do you think its worth trying to make dev dependent on CONFIG_SYSFS?
tavi
^ permalink raw reply
* Re: [PATCH] 3c515: Write outside array bounds
From: Roel Kluin @ 2009-07-29 14:13 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David S. Miller, netdev, Andrew Morton
In-Reply-To: <4A6CC7BD.9020602@gmail.com>
Op 26-07-09 23:16, Jarek Poplawski schreef:
> Roel Kluin wrote, On 07/26/2009 12:35 AM:
>
>> if dev_alloc_skb() fails on the first iteration, a write to
>> cp->rx_ring[-1] occurs.
>>
>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>> ---
>> Please review: can we error return like this?
>
>
> I doubt we can return here: there is a lot of cleaning missing.
>
> Jarek P.
I took drivers/net/3c59x.c as an example
Is this going in the right direction?
diff --git a/drivers/net/3c515.c b/drivers/net/3c515.c
index 3e00fa8..e94867d 100644
--- a/drivers/net/3c515.c
+++ b/drivers/net/3c515.c
@@ -827,7 +827,7 @@ static int corkscrew_open(struct net_device *dev)
skb = dev_alloc_skb(PKT_BUF_SZ);
vp->rx_skbuff[i] = skb;
if (skb == NULL)
- break; /* Bad news! */
+ goto error; /* Bad news! */
skb->dev = dev; /* Mark as being used by this device. */
skb_reserve(skb, 2); /* Align IP on 16 byte boundaries */
vp->rx_ring[i].addr = isa_virt_to_bus(skb->data);
@@ -864,6 +864,17 @@ static int corkscrew_open(struct net_device *dev)
ioaddr + EL3_CMD);
return 0;
+error:
+ pr_emerg("%s: no memory for rx ring\n", dev->name);
+ int j;
+ for (j = 0; j < i; j++) {
+ if (vp->rx_skbuff[j]) {
+ dev_kfree_skb(vp->rx_skbuff[j]);
+ vp->rx_skbuff[j] = NULL;
+ }
+ }
+ free_irq(dev->irq, dev);
+ return -ENOMEM;
}
static void corkscrew_timer(unsigned long data)
^ permalink raw reply related
* Re: tty_register_device NULL pointer dereference in 2.6.31-rc4
From: Oliver Hartkopp @ 2009-07-29 14:00 UTC (permalink / raw)
To: Dave Young; +Cc: Alan Cox, Marcel Holtmann, Linux Netdev List, linux-bluetooth
In-Reply-To: <20090727140736.GA1864@darkstar>
[-- Attachment #1: Type: text/plain, Size: 1607 bytes --]
Dave Young wrote:
> On Mon, Jul 27, 2009 at 01:39:03PM +0200, Oliver Hartkopp wrote:
>> Oliver Hartkopp wrote:
>>> Dave Young wrote:
>>>> On Sat, Jul 25, 2009 at 01:10:46PM +0100, Alan Cox wrote:
>>>>>>> tty_register_device appears to have been called with a NULL pointer. Not
>>>>>>> sure why however.
>>>>>> if that is the pointer for the struct device, then that used to be fine
>>>>>> in the past. Not all RFCOMM device have a parent when they are created.
>>>>> The tty layer doesn't care about the struct device really. Nothing there
>>>>> has changed. The NULL passed appears to be the driver argument.
>>>> Agree with you, because in rfcomm_init, rfcomm thread run before tty initilized, the following patch may fix the problem.
>>>> oliver, could you verify it it fix your problem?
>>> Hi Dave,
>>>
>>> i get this problem really seldom on my Laptop and i did not manage to get a
>>> reproducible Oops of that problem.
>>>
>>> Anyway the code you are pointing to seems to have a problem and your added
>>> error handling looks good to me - even if i don't know if the initializations
>>> can be reordered in that way.
>>>
>>> I'll try your patch, but it could take a *long* time to prove it right ;-)
>>>
>> Just FYI:
>>
>> Your patch compiled, the system booted without problems and nothing is broken
>> so far. I checked the BT, WLAN and BT dial-up with success. So it looks good
>> to me.
>
> Thanks for the testing. Marcel, could you take a look?
Hi Dave,
i got it again - even with your patch (that's why it's 2.6.31-rc4-dirty in the
attached screenshot).
Sorry ;-)
Best regards,
Oliver
[-- Attachment #2: tty_register_device_2-6-31-rc4-patched-by-dave.jpg --]
[-- Type: image/jpeg, Size: 127030 bytes --]
^ permalink raw reply
* Re: [PATCH 5/5] c/r: Add AF_UNIX support (v6)
From: Serge E. Hallyn @ 2009-07-29 13:36 UTC (permalink / raw)
To: Oren Laadan; +Cc: Dan Smith, containers, netdev, Alexey Dobriyan
In-Reply-To: <4A6F6B19.9010508@librato.com>
Quoting Oren Laadan (orenl@librato.com):
> > OL> Does the following bypass security checks for sys_connect() ?
[ on sock_unix_restore()->sock_unix_restore_connected()->sock_unix_join() ]
> >
> > I don't think so. We're basically replicating sys_socketpair() here,
> > which does not do a security check, presumably because all you're
> > doing is hooking two sockets together that both belong to you. That's
> > not to say that we're as safe as that limited operation, but I don't
> > think it's totally clear. Perhaps someone more confident will
> > comment.
>
> Yes, please ... Serge ?
>
> To me it sounds plausible. If we adopt it, then a comment in the
> code is worthwhile.
I'm not sure what Oren means "sounds plausible" or should be adopted.
Using a common helper with sys_connect()?
At the moment you miss out on the security_socket_connect() call. That
may be not as important for unix sockets, but it does look like selinux +
netlabel can label unix sockets as well. So I'm not convinced we can
just ignore it, as once we start properly LSM-labeling tasks and
sockets we may need to do that to ensure proper restart under selinux.
The other thing is that some new fancy doohicky might require another
hook in sys_connect, which may or may not be needed for this path.
If coded this way, we may not find out until someone reports some
subtle failure long after the fact.
Still your code is so customized that perhaps an explicit
security_socket_connect() call in your sock_unix_join() may be the
way to go...
-serge
^ permalink raw reply
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: Krishna Kumar2 @ 2009-07-29 13:28 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, jarkao2, netdev
In-Reply-To: <20090729004428.GA765@gondor.apana.org.au>
Hi Herbert,
Herbert Xu <herbert@gondor.apana.org.au> wrote on 07/29/2009 06:14:28 AM:
>
> Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
>
> On Tue, Jul 28, 2009 at 12:59:19PM -0700, David Miller wrote:
> >
> > The premise is that there'd be only one. The qdisc lock.
> >
> > If the traffic is distributed, flow wise, the driver XMIT
> > lock would spread due to multiqueue.
>
> Suppose that we have a single large flow going through that has
> filled up the hardware queue and is now backlogged in the qdisc
> with qdisc_run on CPU A. Now some other flow comes along and
> sends a packet on CPU B.
>
> So now CPU A and B will both be processing packets for the first
> flow causing loads of lock contention.
>
> But worse yet, we have introduced packet reordering. So are you
> convinced now :)
I am probably misunderstanding you, but ... For different flows, is
this an issue? The same TCP connection will always use a single TXQ,
even if it runs on different CPUs since dev_pick_tx will select the
same txq, so how will reordering happen?
thanks,
- KK
(I am off tomorrow, but will be around for the next 3 hours)
^ permalink raw reply
* Re: [PATCH] eexpress: Read buffer overflow
From: Roel Kluin @ 2009-07-29 13:18 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, netdev, akpm
In-Reply-To: <20090729122953.GC5490@ff.dom.local>
start_code is 69 words, but the code always writes a multiple of 16 words,
so the last 11 words written are outside the array.
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
> Now you seem to make my previous math working :-)
>
>>> (max) i = 64, (max) j = 14, (64+14+16)/2 = 47 < 69, so it seems to copy
>>> less than its size?
>
> Jarek P.
>
You're right, thanks for reviewing, this one should be correct.
diff --git a/drivers/net/eexpress.c b/drivers/net/eexpress.c
index 1686dca..1f016d6 100644
--- a/drivers/net/eexpress.c
+++ b/drivers/net/eexpress.c
@@ -1474,13 +1474,13 @@ static void eexp_hw_init586(struct net_device *dev)
outw(0x0000, ioaddr + 0x800c);
outw(0x0000, ioaddr + 0x800e);
- for (i = 0; i < (sizeof(start_code)); i+=32) {
+ for (i = 0; i < ARRAY_SIZE(start_code) * 2; i+=32) {
int j;
outw(i, ioaddr + SM_PTR);
- for (j = 0; j < 16; j+=2)
+ for (j = 0; j < 16 && (i+j)/2 < ARRAY_SIZE(start_code); j+=2)
outw(start_code[(i+j)/2],
ioaddr+0x4000+j);
- for (j = 0; j < 16; j+=2)
+ for (j = 0; j < 16 && (i+j+16)/2 < ARRAY_SIZE(start_code); j+=2)
outw(start_code[(i+j+16)/2],
ioaddr+0x8000+j);
}
^ permalink raw reply related
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: Krishna Kumar2 @ 2009-07-29 13:08 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, Herbert Xu, netdev
In-Reply-To: <20090729124734.GD5490@ff.dom.local>
Hi Jarek,
Jarek Poplawski <jarkao2@gmail.com> wrote on 07/29/2009 06:17:34 PM:
> Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
>
> On Wed, Jul 29, 2009 at 08:30:41PM +0800, Herbert Xu wrote:
> > On Wed, Jul 29, 2009 at 11:26:14AM +0000, Jarek Poplawski wrote:
> > >
> > > If you mean the tx lock there should be no "real" contention: only
> > > one waiter max. qdisc lock's contention might be higher, but it's
> > > use (during contention) better: enqueue + dequeue together instead
> > > of doing it separately.
> >
> > Hmm, you will have contention if they're both transmitting a
> > single flow which must always go into a single physical queue.
> >
> > So you'll have two CPUs doing the work of a single CPU, with one
> > of them always spinning on the TX lock.
>
> Hmm.. I'd call it a little waiting, but OK let's call it contention;-)
> When tx is faster than queue operations there could be no contention
> at all. I'm not saying I must be right: IMHO it's only worth trying.
My expectation is that tx would be much longer than a few lines of
queue operation....
thanks,
- KK
^ 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