* Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf
From: Linus Torvalds @ 2010-03-06 23:48 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, Nick Andrew, linux-kernel, Greg Kroah-Hartman,
netdev
In-Reply-To: <alpine.LFD.2.00.1003061540470.23804@localhost.localdomain>
On Sat, 6 Mar 2010, Linus Torvalds wrote:
>
> width: 5 (make it 6 bits and signed)
> precision: 5 (make it 6 bits, and signed)
Oh, actually, no, we do that whole left-justification with the flags, we
don't need that signed stuff. We do need one special value for the "no
width, no precision", though.
Linus
^ permalink raw reply
* Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf
From: Linus Torvalds @ 2010-03-06 23:46 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, Nick Andrew, linux-kernel, Greg Kroah-Hartman,
netdev
In-Reply-To: <1267918554.849.89.camel@Joe-Laptop.home>
On Sat, 6 Mar 2010, Joe Perches wrote:
>
> I believe 32 bits isn't possible.
>
> type:6
> flags:8
> width:8
> base:6
> precision:6
> qualifier:8
type: 5
flags: 7
width: 5 (make it 6 bits and signed)
base: 5
precision: 5 (make it 6 bits, and signed)
qualifier: 3
Now, admittedly, right now 'qualifier' really has to be 8, but that's just
because we use an ASCII character for it. But I don't think there are more
than 8 actual legal qualifiers. And 'base' could be just 2 bits, since
there are only a couple of legal bases, but I left room for them all
anyway.
And the other really should fit in those numbers unchanged, I think.
Linus
^ permalink raw reply
* Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf
From: Joe Perches @ 2010-03-06 23:35 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrew Morton, Nick Andrew, linux-kernel, Greg Kroah-Hartman,
netdev
In-Reply-To: <alpine.LFD.2.00.1003061455590.31447@localhost.localdomain>
On Sat, 2010-03-06 at 14:57 -0800, Linus Torvalds wrote:
>
> On Sat, 6 Mar 2010, Linus Torvalds wrote:
> >
> > I'm not convinced. We pass that 'printf_spec' around a lot, including
> > nesting. Not as a pointer, either.
>
> Btw, I wonder if those fields could become bitfields, or 'unsigned char'
> etc. That printf_spec really is unnecessarily big. It should _easily_ fit
> in a single register on a 64-bit platform, possibly even a 32-bit one (we
> could limit field width and precision to 5 bits, for example)
I believe 32 bits isn't possible.
type:6
flags:8
width:8
base:6
precision:6
qualifier:8
So maybe something like this:
lib/vsprintf.c | 22 ++++++++++++----------
1 files changed, 12 insertions(+), 10 deletions(-)
---
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index af4aaa6..fdee7f7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -408,12 +408,12 @@ enum format_type {
};
struct printf_spec {
- enum format_type type;
- int flags; /* flags to number() */
- int field_width; /* width of output field */
- int base;
- int precision; /* # of digits/chars */
- int qualifier;
+ u16 type;
+ s16 field_width; /* width of output field */
+ u8 flags; /* flags to number() */
+ u8 base;
+ s8 precision; /* # of digits/chars */
+ u8 qualifier;
};
static char *number(char *buf, char *end, unsigned long long num,
@@ -1333,7 +1333,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
break;
case FORMAT_TYPE_NRCHARS: {
- int qualifier = spec.qualifier;
+ u8 qualifier = spec.qualifier;
if (qualifier == 'l') {
long *ip = va_arg(args, long *);
@@ -1619,7 +1619,7 @@ do { \
case FORMAT_TYPE_NRCHARS: {
/* skip %n 's argument */
- int qualifier = spec.qualifier;
+ u8 qualifier = spec.qualifier;
void *skip_arg;
if (qualifier == 'l')
skip_arg = va_arg(args, long *);
@@ -1885,7 +1885,9 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
char *next;
char digit;
int num = 0;
- int qualifier, base, field_width;
+ u8 qualifier;
+ u8 base;
+ s16 field_width;
bool is_sign;
while (*fmt && *str) {
@@ -1963,7 +1965,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
{
char *s = (char *)va_arg(args, char *);
if (field_width == -1)
- field_width = INT_MAX;
+ field_width = SHORT_MAX;
/* first, skip leading white space in buffer */
str = skip_spaces(str);
^ permalink raw reply related
* Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf
From: Linus Torvalds @ 2010-03-06 22:57 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, Nick Andrew, linux-kernel, Greg Kroah-Hartman,
netdev
In-Reply-To: <alpine.LFD.2.00.1003061445490.31447@localhost.localdomain>
On Sat, 6 Mar 2010, Linus Torvalds wrote:
>
> I'm not convinced. We pass that 'printf_spec' around a lot, including
> nesting. Not as a pointer, either.
Btw, I wonder if those fields could become bitfields, or 'unsigned char'
etc. That printf_spec really is unnecessarily big. It should _easily_ fit
in a single register on a 64-bit platform, possibly even a 32-bit one (we
could limit field width and precision to 5 bits, for example)
Linus
^ permalink raw reply
* Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf
From: Linus Torvalds @ 2010-03-06 22:52 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, Nick Andrew, linux-kernel, Greg Kroah-Hartman,
netdev
In-Reply-To: <1267914654.849.81.camel@Joe-Laptop.home>
On Sat, 6 Mar 2010, Joe Perches wrote:
>
> Maybe limit the %pV recursion depth to 1 with something like:
> (in vsprintf.c: pointer() )
Nope. Think about concurrent users.
The thing is, you have to hide it in local storage. We could make it
thread-local (not cpu-local), but even that interacts badly with
interrupts.
The only really workable approach would be to have a stack slot that is
created by the externally visible routines (and initialized to zero), and
those then passe the address of that as an argument to the lower levels,
and then the recursion happens entirely within those lower level functions
that update the value.
So it's doable, it's just not pretty.
> > and I'd also love to see some actual numbers of > > how deep the vsnprintf stack frame is, but I don't see how to do the
> > first, and I'm hoping the second isn't too horrible.
>
> I believe it's the arguments, a long long, a couple of pointers,
> and a struct printf_spec. Not too bad.
I'm not convinced. We pass that 'printf_spec' around a lot, including
nesting. Not as a pointer, either.
(Bjorn Helgaas has a patch that gets rid of _some_ of the stack usage, but
not nearly all).
Linus
^ permalink raw reply
* Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf
From: Joe Perches @ 2010-03-06 22:30 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrew Morton, Nick Andrew, linux-kernel, Greg Kroah-Hartman,
netdev
In-Reply-To: <alpine.LFD.2.00.1003061356420.31447@localhost.localdomain>
On Sat, 2010-03-06 at 14:03 -0800, Linus Torvalds wrote:
> On Sat, 6 Mar 2010, Joe Perches wrote:
> > Is that an ack, a nack or a get the hell out?
> It's mostly an Ack. I think the concept is great. I'd love to have some
> way to limit recursion,
Maybe limit the %pV recursion depth to 1 with something like:
(in vsprintf.c: pointer() )
case 'V': {
char *rtn;
static int rcount++;
if (rcount > 1) { /* Don't recurse more than once */
rcount--;
break; /* Use print the pointer */
}
rtn = buf + vsnprintf(buf, end - buf,
((struct va_format *)ptr)->fmt,
*(((struct va_format *)ptr)->va));
rcount--;
return rtn;
}
> and I'd also love to see some actual numbers of
> how deep the vsnprintf stack frame is, but I don't see how to do the
> first, and I'm hoping the second isn't too horrible.
I believe it's the arguments, a long long, a couple of pointers,
and a struct printf_spec. Not too bad.
^ permalink raw reply
* Re: [PATCH 2/6] C/R: Basic support for network namespaces and devices (v5)
From: Oren Laadan @ 2010-03-06 22:21 UTC (permalink / raw)
To: Dan Smith
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
containers-qjLDD68F18O7TbgM5vRIOg, den-GEFAQzZX7r8dnm+yROfE0A,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, benjamin.thery-6ktuUTfB/bM
In-Reply-To: <87bpf1idic.fsf-FLMGYpZoEPULwtHQx/6qkW3U47Q5hpJU@public.gmane.org>
Dan Smith wrote:
> OL> What about leak detection ?
> OL> Aren't we missing {netns,netdev}_users()?
>
> This is something I need to give more thought to, but it's not as easy
> as it sounds. Network devices aren't released at the last put() like
> a lot of other things, and my initial attempts to reconcile the
> refcount after a checkpoint operation have not been successful.
>
> However, I'm not sure that it's as important here, because AFAIK, a
> network device can only exist in one network namespace at a time. If
> we're checkpointing a netdev, it's because we are checkpointing the
> namespace that it lives in. Making sure the netns isn't leaked out of
> the process tree would be much easier and just as effective, no?
We should guarantee that neither netns nor netdev leaks outside
the container; currently none is. If a netdev can only belong to
a single netns, then it suffices to only care about netns.
>
>>> +config CHECKPOINT_NETNS
>>> + bool
>>> + default y if NET && NET_NS && CHECKPOINT
>>> +
>
> OL> Did you mean this to be visible (settable) by the user ?
>
> No, it was specifically supposed to enable itself when those other
> items are enabled, but not be a user adjustable toggle. I had a
> discussion with Serge about it and we came to this as a solution,
> although I don't remember what the problem we started with was. I'll
> dig through my IRC logs to see if I can figure it out.
Duh.. my bad, I misinterpreted the code. That's fine.
BTW, there is a similar SYSVIPC_CHECKPOINT - we should decide
if we do X_CHECKPOINT or CHECKPOINT_X for a subsystem X, and
stick to that convention. I prefer the latter - what you did...
>
>>> + retry:
>>> + if (++pages > 4) {
>>> + addrs = -E2BIG;
>>> + goto out;
>>> + }
>
> OL> Why 4 ?
>
> It's just a sanity limit.
Hmm... let me be more explicit: why not keep trying until it
realloc fails ? or switch to vmalloc() at some point ?
>
> OL> Do we really need this special case ? I'd be happy with a ckpt_err()
> OL> for any error - and the actual error number would be useful to tell
> OL> which case it was.
>
> Unless I'm missing something, you asked for this specifically:
>
> https://lists.linux-foundation.org/pipermail/containers/2010-February/022844.html
Lol .. that was me :o But looking at the code it feels wrong,
because the errno already reveals the type of the problem.
I'm thinking - wouldn't it make sense to do error reporting
in checkpoint_netdev() if the call to ->ndo_checkpoint() fails ?
>
> OL> Isn't this check redundant ? I expect it to fail promptly in
> OL> checkpoint_netdev() above.
>
> No, as I've said a couple of times previously, this isn't the only way
> we can arrive at a netdev for checkpointing. This case is the one
> where we're marching through the netns and find a netdev that is not
> supported. The other is where we arrive at a device as a peer of
> another device, so the other check may come into play at times where
> this one doesn't and vice versa.
I'm confused: in checkpoint_ns() inside the for_each_netdev()
loop you first test for dev->netdev_ops->ndo_checkpoint and
then call checkpoint_obj(... CKPT_OBJ_NETDEV) - which in turn
will call checkpoint_netdev(), which will again test for
dev->netdev_ops->ndo_checkpoint ... am I reading it wrongly ?
>
> OL> This may be a bit simpler if you move the first deferqueue_add()
> OL> forward to just before the other one. Or better: change dq_netdev
> OL> to have two pointers, dev and peer (if any is null, the cleanup
> OL> function will skip).
>
> The reason it is this messy is because of the way network devices are
> deallocated. Since they don't destroy themselves on the final put(),
> we have to explicitly call unregister_netdev() on them when we know
> they're no longer used (else we block). Once we've added them to the
> deferqueue, we can no longer destroy them here because a reference is
> held and the deferqueue will run afterwards.
>
> The ordering of this is a result of me injecting failures at each step
> and working it out until I got it to not block on unregistering either
> of the devices in all of the error paths. That's not to say it's the
> best way, but there is a reason it's ordered the way it is.
>
How about this - to me it feels simpler:
dev = rtnl_newlink(veth_new_link_msg, &veth, this_name);
if (IS_ERR(dev))
return dev;
peer = dev_get_by_name(current->nsproxy->net_ns, peer_name);
if (!peer) {
ret = -EINVAL;
goto err_dev;
}
ret = ckpt_obj_insert(ctx, peer, h->veth.peer_ref,
CKPT_OBJ_NETDEV);
if (ret < 0)
goto err_peer;
dev_put(peer);
dq.dev = dev;
dq.peer = peer;
ret = deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
netdev_noop, netdev_cleanup);
if (ret)
goto err_peer;
(yes, you need to adjust struct dq_netdev and netdev_cleanup).
BTW, the variable "didreg" should disappear from restore_veth().
Oren.
^ permalink raw reply
* [PATCH kernel 2.6.33-git11] lib8390: use spin_lock_irqsave for locking
From: Ken Kawasaki @ 2010-03-06 22:02 UTC (permalink / raw)
To: netdev
In-Reply-To: <20091213094411.18be2ff6.ken_kawasaki@spring.nifty.jp>
lib8390:
use "spin_lock_irqsave", "local_irq_save" instead of "disable_irq".
Signed-off-by: Ken Kawasaki <ken_kawasaki@spring.nifty.jp>
---
--- linux-2.6.33-git11/drivers/net/lib8390.c.orig 2010-03-06 20:09:28.000000000 +0900
+++ linux-2.6.33-git11/drivers/net/lib8390.c 2010-03-07 06:42:47.000000000 +0900
@@ -279,15 +279,13 @@ static void __ei_tx_timeout(struct net_d
/* Ugly but a reset can be slow, yet must be protected */
- disable_irq_nosync_lockdep(dev->irq);
- spin_lock(&ei_local->page_lock);
+ spin_lock_irqsave(&ei_local->page_lock, flags);
/* Try to restart the card. Perhaps the user has fixed something. */
ei_reset_8390(dev);
__NS8390_init(dev, 1);
- spin_unlock(&ei_local->page_lock);
- enable_irq_lockdep(dev->irq);
+ spin_unlock_irqrestore(&ei_local->page_lock, flags);
netif_wake_queue(dev);
}
@@ -323,17 +321,11 @@ static netdev_tx_t __ei_start_xmit(struc
spin_lock_irqsave(&ei_local->page_lock, flags);
ei_outb_p(0x00, e8390_base + EN0_IMR);
- spin_unlock_irqrestore(&ei_local->page_lock, flags);
-
/*
* Slow phase with lock held.
*/
- disable_irq_nosync_lockdep_irqsave(dev->irq, &flags);
-
- spin_lock(&ei_local->page_lock);
-
ei_local->irqlock = 1;
/*
@@ -368,8 +360,7 @@ static netdev_tx_t __ei_start_xmit(struc
ei_local->irqlock = 0;
netif_stop_queue(dev);
ei_outb_p(ENISR_ALL, e8390_base + EN0_IMR);
- spin_unlock(&ei_local->page_lock);
- enable_irq_lockdep_irqrestore(dev->irq, &flags);
+ spin_unlock_irqrestore(&ei_local->page_lock, flags);
dev->stats.tx_errors++;
return NETDEV_TX_BUSY;
}
@@ -409,8 +400,7 @@ static netdev_tx_t __ei_start_xmit(struc
ei_local->irqlock = 0;
ei_outb_p(ENISR_ALL, e8390_base + EN0_IMR);
- spin_unlock(&ei_local->page_lock);
- enable_irq_lockdep_irqrestore(dev->irq, &flags);
+ spin_unlock_irqrestore(&ei_local->page_lock, flags);
dev_kfree_skb (skb);
dev->stats.tx_bytes += send_length;
@@ -526,9 +516,11 @@ static irqreturn_t __ei_interrupt(int ir
#ifdef CONFIG_NET_POLL_CONTROLLER
static void __ei_poll(struct net_device *dev)
{
- disable_irq(dev->irq);
+ unsigned long flags;
+
+ local_irq_save(flags);
__ei_interrupt(dev->irq, dev);
- enable_irq(dev->irq);
+ local_irq_restore(flags);
}
#endif
^ permalink raw reply
* Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf
From: Linus Torvalds @ 2010-03-06 22:03 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, Nick Andrew, linux-kernel, Greg Kroah-Hartman,
netdev
In-Reply-To: <1267911399.849.39.camel@Joe-Laptop.home>
On Sat, 6 Mar 2010, Joe Perches wrote:
>
> Is that an ack, a nack or a get the hell out?
It's mostly an Ack. I think the concept is great. I'd love to have some
way to limit recursion, and I'd also love to see some actual numbers of
how deep the vsnprintf stack frame is, but I don't see how to do the
first, and I'm hoping the second isn't too horrible.
Linus
^ permalink raw reply
* Re: [tcpdump-workers] Current wireless-testing breaks libpcap: mr_alen should be set
From: Guy Harris @ 2010-03-06 21:23 UTC (permalink / raw)
To: tcpdump-workers; +Cc: linux-wireless, netdev, Jiri Pirko
In-Reply-To: <1267578048.14049.11.camel@mj>
On Mar 2, 2010, at 5:00 PM, Pavel Roskin wrote:
> This patch to libpcap helps:
>
> --- a/pcap-linux.c
> +++ b/pcap-linux.c
> @@ -1563,6 +1563,7 @@ live_open_new(pcap_t *handle, const char
> memset(&mr, 0, sizeof(mr));
> mr.mr_ifindex = handle->md.ifindex;
> mr.mr_type = PACKET_MR_PROMISC;
> + mr.mr_alen = 6;
If there are any network types that support promiscuous mode and have link-layer addresses that aren't 6 octets long, that would still fail.
It sounds as if the fix is not to care about the address length if the address isn't used, so you don't need to get the length right for PACKET_MR_PROMISC or PACKET_MR_ALLMULTI, so libpcap, and other clients setting promiscuous or "show me all multicast packets" mode, don't need to change. Is that the case?
^ permalink raw reply
* Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf
From: Joe Perches @ 2010-03-06 21:36 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrew Morton, Nick Andrew, linux-kernel, Greg Kroah-Hartman,
netdev
In-Reply-To: <alpine.LFD.2.00.1003041504080.3751@localhost.localdomain>
On Thu, 2010-03-04 at 15:06 -0800, Linus Torvalds wrote:
> On Thu, 4 Mar 2010, Andrew Morton wrote:
> > What would I need to do to make it recur more than once? Include a %pV
> > in a string, like dev_printk("%s", %%pV")?
>
> I would argue that if somebody does that, they're just broken.
>
> The single level of recursion is really nice - it solves a real annoyance.
> But if somebody starts trying to do multiple levels, that's just crazy.
>
> Of course, I guess we don't relly have any way of sanely _counting_ the
> recursion level, so we'd have to just trust people to not do crazy things.
>
> And that's a big thing to take on trust ;)
Is that an ack, a nack or a get the hell out?
^ permalink raw reply
* Re: [RFC][PATCH] ns: Syscalls for better namespace sharing control.
From: Daniel Lezcano @ 2010-03-06 21:26 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Pavel Emelyanov, Linux Netdev List,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
Netfilter Development Mailinglist, Ben Greear,
Sukadev Bhattiprolu
In-Reply-To: <m1aaulyy5c.fsf-+imSwln9KH6u2/kzUuoCbdi2O/JbrIOy@public.gmane.org>
Eric W. Biederman wrote:
> Daniel Lezcano <daniel.lezcano-GANU6spQydw@public.gmane.org> writes:
>
>
>> Eric W. Biederman wrote:
>>
>
>
>> If the normal rules of parentage apply, that means pid 0 has to wait it's child.
>> If we are in the scenario of pid 0, it's child pid 1234 and we kill the pid 1 of
>> the pid namespace, I suppose pid 1234 will be killed too.
>> The pid 0 will stay in the pid namespace and will able to fork again a new pid
>> 1.
>>
>> I think Serge already reported that...
>>
>> That sounds good :)
>>
>
> I expect zap_pid_ns_processes should also arrange so we cannot allocate any
> more processes. We certainly need to do something explicit or pid 1 won't
> be allocated. It might make sense to resurrect a pid namespace after it's
> death but it is definitely weird.
>
Mmh, yes. But that was just an idea, maybe a bit out of the scope you
are aiming.
>>> In a lot of ways I like this idea of sys_hijack/sys_cloneat, and I
>>> don't think anything I am doing fundamentally undermines it. The use
>>> case of doing things in fork is that there is automatic inheritance of
>>> everything. All of the namespaces and all of the control groups, and
>>> possibly also the parent process.
>>>
>> And also the rootfs for executing the command inside the container
>> (eg. shutdown), the uid/gid (if there is a user namespace), the mount points,
>> ...
>> But I suppose we can do the same with setns for all the namespaces and chrooting
>> within the container rootfs.
>>
>> What I see is a problem with the tty. For example, we cloneat the init process
>> of the container which is usually /sbin/init but this one has its tty mapped to
>> /dev/console, so the output of the exec'ed command will go to the console.
>>
>
> My original thinking was that the fd's would come from the caller of sys_cloneat....
Oh, ok :s
>>> Overall it sounds like the semantics I have proposed with
>>> unshare(CLONE_NEWPID) are workable, and simple to implement. The
>>> extra fork is a bit surprising but it certainly does not
>>> look like a show stopper for implementing a pid namespace join.
>>>
>>>
>> I agree, it's some kind of "ghost" process.
>> IMO, with a bit of userspace code it would be possible to enter or exec a
>> command inside a container with nsfd, setns.
>>
>> +1 to test your patchset Eric :)
>>
>
> I will see about reposting sometime soon.
>
Great ! thanks.
>> Just a mindless suggestion, the "nsopen" / "nsattach" syscall names should be
>> more clear no ?
>>
>
> Not bad suggestions.
>
> I am going to explore a bit more. Given that nsfd is using the same
> permission checks as a proc file, I think I can just make it a proc
> file. Something like "/proc/<pid>/ns/net". With a little luck that
> won't suck too badly.
>
Ah ! yes. Good idea.
^ permalink raw reply
* Re: Problems with remote-wakeup settings
From: Rafael J. Wysocki @ 2010-03-06 21:25 UTC (permalink / raw)
To: Alan Stern
Cc: NetDev, USB list, linux-hotplug, Kernel development list,
linux-input, Linux-pm mailing list
In-Reply-To: <Pine.LNX.4.44L0.1003061601520.23895-100000@netrider.rowland.org>
On Saturday 06 March 2010, Alan Stern wrote:
> On Sat, 6 Mar 2010, Rafael J. Wysocki wrote:
>
> > > It's not difficult in theory to tie together the WoL setting and the
> > > wakeup flag:
> > >
> > > If ethtool changes the WoL setting, the driver's ioctl handler
> > > should make the corresponding change to the wakeup flag.
> > >
> > > If ethtool queries the WoL setting, the ioctl handler should
> > > check the wakeup flag. If the flag is off, it should report
> > > that WoL is disabled; if the flag is on, it should report that
> > > WoL is enabled. (The same check should be made in the suspend
> > > routine.)
> >
> > That's done this way already in all drivers I know, but we need a hook
> > from wake_store() back to the driver.
>
> What for? wake_store() can't be called during a sleep transition
> (because tasks are frozen) or while the system is asleep. And if it is
> called at any other time, the driver doesn't need to know until either
> its ioctl handler or its suspend method runs.
Right.
That means, though, that the network adapter drivers' "get WoL" routines
should check should_wakeup too. They don't do that right now, but IMO it's
reasonable to request that they be modified.
Adding netdev to the Cc list.
Rafael
^ permalink raw reply
* Re: [RFC][PATCH] ns: Syscalls for better namespace sharing control.
From: Eric W. Biederman @ 2010-03-06 20:48 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Pavel Emelyanov, Linux Netdev List,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
Netfilter Development Mailinglist, Ben Greear,
Sukadev Bhattiprolu
In-Reply-To: <4B926B1B.5070207-GANU6spQydw@public.gmane.org>
Daniel Lezcano <daniel.lezcano-GANU6spQydw@public.gmane.org> writes:
> Eric W. Biederman wrote:
> If the normal rules of parentage apply, that means pid 0 has to wait it's child.
> If we are in the scenario of pid 0, it's child pid 1234 and we kill the pid 1 of
> the pid namespace, I suppose pid 1234 will be killed too.
> The pid 0 will stay in the pid namespace and will able to fork again a new pid
> 1.
>
> I think Serge already reported that...
>
> That sounds good :)
I expect zap_pid_ns_processes should also arrange so we cannot allocate any
more processes. We certainly need to do something explicit or pid 1 won't
be allocated. It might make sense to resurrect a pid namespace after it's
death but it is definitely weird.
>> In a lot of ways I like this idea of sys_hijack/sys_cloneat, and I
>> don't think anything I am doing fundamentally undermines it. The use
>> case of doing things in fork is that there is automatic inheritance of
>> everything. All of the namespaces and all of the control groups, and
>> possibly also the parent process.
> And also the rootfs for executing the command inside the container
> (eg. shutdown), the uid/gid (if there is a user namespace), the mount points,
> ...
> But I suppose we can do the same with setns for all the namespaces and chrooting
> within the container rootfs.
>
> What I see is a problem with the tty. For example, we cloneat the init process
> of the container which is usually /sbin/init but this one has its tty mapped to
> /dev/console, so the output of the exec'ed command will go to the console.
My original thinking was that the fd's would come from the caller of sys_cloneat....
>> Overall it sounds like the semantics I have proposed with
>> unshare(CLONE_NEWPID) are workable, and simple to implement. The
>> extra fork is a bit surprising but it certainly does not
>> look like a show stopper for implementing a pid namespace join.
>>
> I agree, it's some kind of "ghost" process.
> IMO, with a bit of userspace code it would be possible to enter or exec a
> command inside a container with nsfd, setns.
>
> +1 to test your patchset Eric :)
I will see about reposting sometime soon.
> Just a mindless suggestion, the "nsopen" / "nsattach" syscall names should be
> more clear no ?
Not bad suggestions.
I am going to explore a bit more. Given that nsfd is using the same
permission checks as a proc file, I think I can just make it a proc
file. Something like "/proc/<pid>/ns/net". With a little luck that
won't suck too badly.
> Jumping back, one question about the nsfd and the poll for waiting the end of
> the namespace.
> If we have an openened file descriptor on a specific namespace, we grab a
> reference on this one, so the namespace won't be destroyed until we close the fd
> which is used to poll the end of the namespace, no ? Did I miss something ?
Not really. The assumption was that there would be a very similar
file descriptor that we could use with poll.
Eric
^ permalink raw reply
* Re: [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks
From: John Fastabend @ 2010-03-06 19:27 UTC (permalink / raw)
To: Herbert Xu
Cc: David Miller, Kirsher, Jeffrey T, netdev@vger.kernel.org,
gospo@redhat.com
In-Reply-To: <20100227155245.GB3176@gondor.apana.org.au>
Herbert Xu wrote:
> On Sat, Feb 27, 2010 at 03:27:25AM -0800, David Miller wrote:
>
>> If we have ip_summed == CHECKSUM_PARTIAL might some classifier
>> or packet scheduler action module require that the
>> transport header is setup properly before the SKB gets into
>> there?
>>
>
> I think this is OK as the transport header setting was only there
> for backwards compatibility with certain drivers that relied on it.
> Its use was very much isolated.
>
> I just did a grep on net/sched and couldn't see anything obvious
> that uses transport_header.
>
>
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index eb7f1a4..626124d 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -1835,12 +1835,40 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>>> {
>>> const struct net_device_ops *ops = dev->netdev_ops;
>>> int rc = NETDEV_TX_OK;
>>> + int need_gso = netif_needs_gso(dev, skb);
>>> +
>>> + if (!need_gso) {
>>> + if (skb_has_frags(skb) &&
>>> + !(dev->features & NETIF_F_FRAGLIST) &&
>>> + __skb_linearize(skb))
>>> + goto out_kfree_skb;
>>> +
>>> + /* Fragmented skb is linearized if device does not support SG,
>>> + * or if at least one of fragments is in highmem and device
>>> + * does not support DMA from it.
>>> + */
>>> + if (skb_shinfo(skb)->nr_frags &&
>>> + (!(dev->features & NETIF_F_SG) ||
>>> + illegal_highdma(dev, skb)) &&
>>> + __skb_linearize(skb))
>>> + goto out_kfree_skb;
>>>
>
> Please use skb_needs_linearize.
>
>
>>> + /* If packet is not checksummed and device does not support
>>> + * checksumming for this protocol, complete checksumming here.
>>> + */
>>> + if (skb->ip_summed == CHECKSUM_PARTIAL) {
>>> + skb_set_transport_header(skb, skb->csum_start -
>>> + skb_headroom(skb));
>>> + if (!dev_can_checksum(dev, skb) &&
>>> + skb_checksum_help(skb))
>>> + goto out_kfree_skb;
>>> + }
>>> + }
>>>
>>> if (likely(!skb->next)) {
>>> if (!list_empty(&ptype_all))
>>> dev_queue_xmit_nit(skb, dev);
>>>
>>> - if (netif_needs_gso(dev, skb)) {
>>> + if (need_gso) {
>>> if (unlikely(dev_gso_segment(skb)))
>>> goto out_kfree_skb;
>>> if (skb->next)
>>>
>
> That whole if block should be moved into an else clause here:
>
> if (netif_needs_gso(dev, skb)) {
> if (unlikely(dev_gso_segment(skb)))
> goto out_kfree_skb;
> if (skb->next)
> goto gso;
> } else {
> do your thing
> }
>
> The reason is that the other paths only act on the fragments
> generated by GSO, so logically with your change we shouldn't
> apply any further software emulation to them, even if the device
> setting changed.
>
> Cheers,
>
Herbert,
It looks like dev_gso_segment() could be used to "Verify header
integrity only" according to the comment? If this is true I think the
logic should probably be
if (netif_needs_gso(dev, skb)) {
if (unlikely(dev_gso_segment(skb)))
goto out_kfree_skb;
if (skb->next)
goto gso;
}
do your thing
That way we linearize the skb if necessary in the case were
dev_gso_segment() only verifies the header and does not return a list of
segments.
thanks,
John.
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Paul E. McKenney @ 2010-03-06 19:00 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100306151933.GD6812@linux.vnet.ibm.com>
On Sat, Mar 06, 2010 at 07:19:33AM -0800, Paul E. McKenney wrote:
> On Sat, Mar 06, 2010 at 02:56:55PM +0800, Herbert Xu wrote:
> > On Fri, Mar 05, 2010 at 09:06:56PM -0800, Paul E. McKenney wrote:
> > >
> > > Agreed, but the callbacks registered by the call_rcu_bh() might run
> > > at any time, possibly quite some time after the synchronize_rcu_bh()
> > > completes. For example, the last call_rcu_bh() might register on
> > > one CPU, and the synchronize_rcu_bh() on another CPU. Then there
> > > is no guarantee that the call_rcu_bh()'s callback will execute before
> > > the synchronize_rcu_bh() returns.
> > >
> > > In contrast, rcu_barrier_bh() is guaranteed not to return until all
> > > pending RCU-bh callbacks have executed.
> >
> > You're absolutely right. I'll send a patch to fix this.
> >
> > Incidentally, does rcu_barrier imply rcu_barrier_bh? What about
> > synchronize_rcu and synchronize_rcu_bh? The reason I'm asking is
> > that we use a mixture of rcu_read_lock_bh and rcu_read_lock all
> > over the place but only ever use rcu_barrier and synchronize_rcu.
>
> Hmmm... rcu_barrier() definitely does -not- imply rcu_barrier_bh(),
> because there are separate sets of callbacks whose execution can
> be throttled separately. So, while you would expect RCU-bh grace
> periods to complete more quickly, if there was a large number of
> RCU-bh callbacks on a given CPU but very few RCU callbacks, it might
> well take longer for the RCU-bh callbacks to be invoked.
>
> With TREE_PREEMPT_RCU, if there were no RCU readers but one long-running
> RCU-bh reader, then synchronize_rcu_bh() could return before
> synchronize_rcu() does.
>
> The simple approach would be to do something like:
>
> synchronize_rcu();
> synchronize_rcu_bh();
>
> on the one hand, and:
>
> rcu_barrier();
> rcu_barrier_bh();
>
> on the other. However, this is not so good for update-side latency.
>
> Perhaps we need a primitive that waits for both RCU and RCU-bh in
> parallel? This is pretty easy for synchronize_rcu() and
> synchronize_rcu_bh(), and probably not too hard for rcu_barrier()
> and rcu_barrier_bh().
>
> Hmmm... Do we have the same issue with call_rcu() and call_rcu_bh()?
But before I get too excited...
You really are talking about code like the following, correct?
rcu_read_lock();
p = rcu_dereference(global_p);
do_something_with(p);
rcu_read_unlock();
. . .
rcu_read_lock_bh();
p = rcu_dereference(global_p);
do_something_else_with(p);
rcu_read_unlock_bh();
. . .
spin_lock(&my_lock);
p = global_p;
rcu_assign_pointer(global_p, NULL);
synchronize_rcu(); /* BUG -- also need synchronize_rcu_bh(). */
kfree(p);
spin_unlock(&my_lock);
In other words, different readers traversing the same data structure
under different flavors of RCU protection, but then using only one
flavor of RCU grace period during the update?
Thanx, Paul
> > > > I understand. However, AFAICS whatever it is that we are destroying
> > > > is taken off the reader's visible data structure before call_rcu_bh.
> > > > Do you have a particular case in mind where this is not the case?
> > >
> > > I might simply have missed the operation that removed reader
> > > visibility, looking again...
> > >
> > > Ah, I see it. The "br->mdb = NULL" in br_multicast_stop() makes
> > > it impossible for the readers to get to any of the data. Right?
> >
> > Yes. The read-side will see it and get nothing, while all write-side
> > paths will see that netif_running is false and exit.
> >
> > > > > The br_multicast_del_pg() looks to need rcu_read_lock_bh() and
> > > > > rcu_read_unlock_bh() around its loop, if I understand the pointer-walking
> > > > > scheme correctly.
> > > >
> > > > Any function that modifies the data structure is done under the
> > > > multicast_lock, including br_multicast_del_pg.
> > >
> > > But spin_lock() does not take the place of rcu_read_lock_bh().
> > > And so, in theory, the RCU-bh grace period could complete between
> > > the time that br_multicast_del_pg() does its call_rcu_bh() and the
> > > "*pp = p->next;" at the top of the next loop iteration. If so,
> > > then br_multicast_free_pg()'s kfree() will possibly have clobbered
> > > "p->next". Low probability, yes, but a long-running interrupt
> > > could do the trick.
> > >
> > > Or is there something I am missing that is preventing an RCU-bh
> > > grace period from completing near the bottom of br_multicast_del_pg()'s
> > > "for" loop?
> >
> > Well all the locks are taken with BH disabled, this should prevent
> > this problem, no?
> >
> > > > The read-side is the data path (non-IGMP multicast packets). The
> > > > sole entry point is br_mdb_get().
> > >
> > > Hmmm... So the caller is responsible for rcu_read_lock_bh()?
> >
> > Yes, all data paths through the bridge operate with BH disabled.
> >
> > > Shouldn't the br_mdb_get() code path be using hlist_for_each_entry_rcu()
> > > in __br_mdb_ip_get(), then? Or is something else going on here?
> >
> > Indeed it should, I'll fix this up too.
> >
> > Thanks for reviewing Paul!
> > --
> > 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] [iproute] ip exit, ip and tc line number
From: Michele Petrazzo - Unipex @ 2010-03-06 18:56 UTC (permalink / raw)
To: netdev
In-Reply-To: <20100303163028.21ef4194@nehalam>
[-- Attachment #1: Type: text/plain, Size: 133 bytes --]
Stephen Hemminger wrote:
> The force issue is a different problem so please split out that part
> and resubmit.
Attached.
Michele
[-- Attachment #2: iproute.diff --]
[-- Type: text/x-diff, Size: 900 bytes --]
commit b1875eb9564da302ba79c01746ec9cf391d0d593
Author: Michele Petrazzo <michele.petrazzo@unipex.it>
Date: Sat Mar 6 19:45:27 2010 +0100
Allow ip to process all the file passed with the -batch argument when
is passed also the -force switch
Signed-off-by: Michele Petrazzo <michele.petrazzo@unipex.it>
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index e9256d9..8ec6cfd 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1014,7 +1014,7 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
}
if (l && matches(d, l) != 0) {
fprintf(stderr, "\"dev\" (%s) must match \"label\" (%s).\n", d, l);
- exit(1);
+ return -1;
}
if (peer_len == 0 && local_len) {
@@ -1079,7 +1079,7 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
}
if (rtnl_talk(&rth, &req.n, 0, 0, NULL, NULL, NULL) < 0)
- exit(2);
+ return -2;
return 0;
}
^ permalink raw reply related
* Re: [RFC v2 00/10] snet: Security for NETwork syscalls
From: Samir Bellabes @ 2010-03-06 18:50 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-kernel, netdev, netfilter-devel, hadi, kaber, zbr, nhorman,
root, linux-security-module
In-Reply-To: <201003030156.o231udx1023055@www262.sakura.ne.jp>
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
> Regarding [RFC v2 05/10] snet: introduce snet_event
> +int snet_event_is_registered(const enum snet_syscall syscall, const u8 protocol)
>
> Maybe rcu_read_lock() is better than rw spinlock because this function is
> frequently called.
Indeed. Evgeniy Polyakov already noticed that, it's on my TODO.
thank you Tetsuo
sam
^ permalink raw reply
* Re: [RFC v2 00/10] snet: Security for NETwork syscalls
From: Samir Bellabes @ 2010-03-06 18:47 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-kernel, netdev, netfilter-devel, hadi, kaber, zbr, nhorman,
root, linux-security-module
In-Reply-To: <201003030156.o231udx1023055@www262.sakura.ne.jp>
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
> Regarding [RFC v2 09/10] snet: introduce snet_ticket
> +enum snet_verdict snet_ticket_check(struct snet_info *info)
> +{
> + struct snet_ticket *st = NULL;
> + unsigned int h = 0, verdict = SNET_VERDICT_NONE;
> + struct list_head *l = NULL;
> + struct snet_task_security *tsec = NULL;
> +
> + if (snet_ticket_mode == SNET_TICKET_OFF)
> + goto out;
> +
> + tsec = (struct snet_task_security*) current_security();
> +
> + h = jhash_2words(info->syscall, info->protocol, 0) % HSIZE;
> + l = &tsec->hash[h];
> +
> + read_lock_bh(&tsec->lock);
>
> Credentials are allocated for copy-on-write basis.
> Sharing "tsec" among multiple "struct task_struct" is what you intended?
No, there is no shared "tsec".
snet_ticket_check() is called from the process context. So "tsec" is
a pointer to the "void *security" pointer from its own "struct
task_struct".
every task_struct have a "tsec" allocated to its "void *security"
pointer.
I will take a second look on how to access the credentials COW.
> Regards.
Tetsuo, thank you again for reviewing.
sam
^ permalink raw reply
* Re: [RFC v2 00/10] snet: Security for NETwork syscalls
From: Samir Bellabes @ 2010-03-06 18:40 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-kernel, netdev, netfilter-devel, hadi, kaber, zbr, nhorman,
root, linux-security-module, Serge Hallyn
In-Reply-To: <201003030156.o231udx1023055@www262.sakura.ne.jp>
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
> Regarding [RFC v2 02/10] Revert "lsm: Remove the socket_post_accept() hook"
> @@ -1538,6 +1538,8 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
> fd_install(newfd, newfile);
> err = newfd;
>
> + security_socket_post_accept(sock, newsock);
> +
> out_put:
> fput_light(sock->file, fput_needed);
> out:
>
> Please move security_socket_post_accept() to before fd_install().
> Otherwise, other threads which share fd tables can use
> security-informations-not-yet-updated accept()ed sockets.
Tetsuo, what about this patch ?
>From 6bbae933058e5f0857e9d7c1a720f9d09080474e Mon Sep 17 00:00:00 2001
From: Samir Bellabes <sam@synack.fr>
Date: Fri, 1 Jan 2010 03:24:52 +0100
Subject: [PATCH] lsm: add security_post_accept() hook
snet need to reintroduce this hook, as it was designed to be: a hook for
updating security informations on objects.
This patch is the result of :
Revert "lsm: Remove the socket_post_accept() hook"
the revert of 8651d5c0b1f874c5b8307ae2b858bc40f9f02482.
and the comment of Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> :
> Please move security_socket_post_accept() to before fd_install().
> Otherwise, other threads which share fd tables can use
> security-informations-not-yet-updated accept()ed sockets.
Signed-off-by: Samir Bellabes <sam@synack.fr>
---
include/linux/security.h | 13 +++++++++++++
net/socket.c | 2 ++
security/capability.c | 5 +++++
security/security.c | 5 +++++
4 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 74e564b..d8ad624 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -938,6 +938,11 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
* @sock contains the listening socket structure.
* @newsock contains the newly created server socket for connection.
* Return 0 if permission is granted.
+ * @socket_post_accept:
+ * This hook allows a security module to copy security
+ * information into the newly created socket's inode.
+ * @sock contains the listening socket structure.
+ * @newsock contains the newly created server socket for connection.
* @socket_sendmsg:
* Check permission before transmitting a message to another socket.
* @sock contains the socket structure.
@@ -1674,6 +1679,8 @@ struct security_operations {
struct sockaddr *address, int addrlen);
int (*socket_listen) (struct socket *sock, int backlog);
int (*socket_accept) (struct socket *sock, struct socket *newsock);
+ void (*socket_post_accept) (struct socket *sock,
+ struct socket *newsock);
int (*socket_sendmsg) (struct socket *sock,
struct msghdr *msg, int size);
int (*socket_recvmsg) (struct socket *sock,
@@ -2696,6 +2703,7 @@ int security_socket_bind(struct socket *sock, struct sockaddr *address, int addr
int security_socket_connect(struct socket *sock, struct sockaddr *address, int addrlen);
int security_socket_listen(struct socket *sock, int backlog);
int security_socket_accept(struct socket *sock, struct socket *newsock);
+void security_socket_post_accept(struct socket *sock, struct socket *newsock);
int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size);
int security_socket_recvmsg(struct socket *sock, struct msghdr *msg,
int size, int flags);
@@ -2778,6 +2786,11 @@ static inline int security_socket_accept(struct socket *sock,
return 0;
}
+static inline void security_socket_post_accept(struct socket *sock,
+ struct socket *newsock)
+{
+}
+
static inline int security_socket_sendmsg(struct socket *sock,
struct msghdr *msg, int size)
{
diff --git a/net/socket.c b/net/socket.c
index b4eb361..384e23d 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1533,6 +1533,8 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
goto out_fd;
}
+ security_socket_post_accept(sock, newsock);
+
/* File flags are not inherited via accept() unlike another OSes. */
fd_install(newfd, newfile);
diff --git a/security/capability.c b/security/capability.c
index a9810dc..61eae40 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -641,6 +641,10 @@ static int cap_socket_accept(struct socket *sock, struct socket *newsock)
return 0;
}
+static void cap_socket_post_accept(struct socket *sock, struct socket *newsock)
+{
+}
+
static int cap_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
{
return 0;
@@ -1081,6 +1085,7 @@ void security_fixup_ops(struct security_operations *ops)
set_to_cap_if_null(ops, socket_connect);
set_to_cap_if_null(ops, socket_listen);
set_to_cap_if_null(ops, socket_accept);
+ set_to_cap_if_null(ops, socket_post_accept);
set_to_cap_if_null(ops, socket_sendmsg);
set_to_cap_if_null(ops, socket_recvmsg);
set_to_cap_if_null(ops, socket_getsockname);
diff --git a/security/security.c b/security/security.c
index 288c3a8..673979f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1082,6 +1082,11 @@ int security_socket_accept(struct socket *sock, struct socket *newsock)
return security_ops->socket_accept(sock, newsock);
}
+void security_socket_post_accept(struct socket *sock, struct socket *newsock)
+{
+ security_ops->socket_post_accept(sock, newsock);
+}
+
int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
{
return security_ops->socket_sendmsg(sock, msg, size);
--
1.6.2.5
^ permalink raw reply related
* Re: [RFC v2 00/10] snet: Security for NETwork syscalls
From: Samir Bellabes @ 2010-03-06 18:20 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-kernel, netdev, netfilter-devel, hadi, kaber, zbr, nhorman,
root, linux-security-module
In-Reply-To: <201003030156.o231udx1023055@www262.sakura.ne.jp>
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
> Regarding [RFC v2 06/10] snet: introduce snet_hooks
> + if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
>
> Please avoid assignment in "if" statement, as scripts/checkpatch.pl suggests.
Right, I should have run script/checkpatch.pl on this set of patches. I
won't miss it next time.
Thanks Tetsuo.
sam
>From ea2234c07ea1bf76579ba127c8a8be6ab276515f Mon Sep 17 00:00:00 2001
From: Samir Bellabes <sam@synack.fr>
Date: Sat, 6 Mar 2010 17:49:22 +0100
Subject: [PATCH 3/3] snet: avoid assigment in if statement
Noticed by Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Samir Bellabes <sam@synack.fr>
---
security/snet/snet_hooks.c | 37 +++++++++++++++++++++++++------------
1 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/security/snet/snet_hooks.c b/security/snet/snet_hooks.c
index 689babe..d9cf0b9 100644
--- a/security/snet/snet_hooks.c
+++ b/security/snet/snet_hooks.c
@@ -160,7 +160,8 @@ static int snet_socket_bind(struct socket *sock,
info.dst.u3.ip = inet->inet_daddr;
info.src.u.port = ntohs(a->sin_port);
/* check tickets */
- if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
+ verdict = snet_ticket_check(&info) ;
+ if (verdict != SNET_VERDICT_NONE)
goto out;
/* inserting verdict PENDING */
info.verdict_id = snet_verdict_insert();
@@ -173,7 +174,8 @@ static int snet_socket_bind(struct socket *sock,
sizeof(info.dst.u3.ip6));
info.src.u.port = ntohs(a6->sin6_port);
/* check tickets */
- if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
+ verdict = snet_ticket_check(&info);
+ if (verdict != SNET_VERDICT_NONE)
goto out;
/* inserting verdict PENDING */
info.verdict_id = snet_verdict_insert();
@@ -229,7 +231,8 @@ static int snet_socket_connect(struct socket *sock,
info.dst.u3.ip = a->sin_addr.s_addr;
info.dst.u.port = ntohs(a->sin_port);
/* check tickets */
- if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
+ verdict = snet_ticket_check(&info);
+ if (verdict != SNET_VERDICT_NONE)
goto out;
/* inserting verdict PENDING */
info.verdict_id = snet_verdict_insert();
@@ -242,7 +245,8 @@ static int snet_socket_connect(struct socket *sock,
sizeof(info.dst.u3.ip6));
info.dst.u.port = ntohs(a6->sin6_port);
/* check tickets */
- if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
+ verdict = snet_ticket_check(&info);
+ if (verdict != SNET_VERDICT_NONE)
goto out;
/* inserting verdict PENDING */
info.verdict_id = snet_verdict_insert();
@@ -295,7 +299,8 @@ static int snet_socket_listen(struct socket *sock, int backlog)
info.src.u3.ip = inet->inet_saddr;
info.dst.u3.ip = inet->inet_daddr;
/* check tickets */
- if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
+ verdict = snet_ticket_check(&info);
+ if (verdict != SNET_VERDICT_NONE)
goto out;
/* inserting verdict PENDING */
info.verdict_id = snet_verdict_insert();
@@ -306,7 +311,9 @@ static int snet_socket_listen(struct socket *sock, int backlog)
sizeof(info.src.u3.ip6));
memcpy(&info.dst.u3.ip6, (void *)&inet->pinet6->daddr,
sizeof(info.dst.u3.ip6));
- if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
+ /* check tickets */
+ verdict = snet_ticket_check(&info);
+ if (verdict != SNET_VERDICT_NONE)
goto out;
/* inserting verdict PENDING */
info.verdict_id = snet_verdict_insert();
@@ -358,7 +365,8 @@ static int snet_socket_accept(struct socket *sock, struct socket *newsock)
info.src.u3.ip = inet->inet_saddr;
info.dst.u3.ip = inet->inet_daddr;
/* check tickets */
- if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
+ verdict = snet_ticket_check(&info);
+ if (verdict != SNET_VERDICT_NONE)
goto out;
/* inserting verdict PENDING */
info.verdict_id = snet_verdict_insert();
@@ -370,7 +378,8 @@ static int snet_socket_accept(struct socket *sock, struct socket *newsock)
memcpy(&info.dst.u3.ip6, (void *)&inet->pinet6->daddr,
sizeof(info.dst.u3.ip6));
/* check tickets */
- if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
+ verdict = snet_ticket_check(&info);
+ if (verdict != SNET_VERDICT_NONE)
goto out;
/* inserting verdict PENDING */
info.verdict_id = snet_verdict_insert();
@@ -470,7 +479,8 @@ static int snet_socket_sendmsg(struct socket *sock,
info.src.u3.ip = inet->inet_saddr;
info.dst.u3.ip = inet->inet_daddr;
/* check tickets */
- if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
+ verdict = snet_ticket_check(&info);
+ if (verdict != SNET_VERDICT_NONE)
goto out;
/* inserting verdict PENDING */
info.verdict_id = snet_verdict_insert();
@@ -482,7 +492,8 @@ static int snet_socket_sendmsg(struct socket *sock,
memcpy(&info.dst.u3.ip6, (void *)&inet->pinet6->daddr,
sizeof(info.dst.u3.ip6));
/* check tickets */
- if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
+ verdict = snet_ticket_check(&info);
+ if (verdict != SNET_VERDICT_NONE)
goto out;
/* inserting verdict PENDING */
info.verdict_id = snet_verdict_insert();
@@ -535,7 +546,8 @@ static int snet_socket_recvmsg(struct socket *sock,
info.src.u3.ip = inet->inet_saddr;
info.dst.u3.ip = inet->inet_daddr;
/* check tickets */
- if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
+ verdict = snet_ticket_check(&info);
+ if (verdict != SNET_VERDICT_NONE)
goto out;
/* inserting verdict PENDING */
info.verdict_id = snet_verdict_insert();
@@ -547,7 +559,8 @@ static int snet_socket_recvmsg(struct socket *sock,
memcpy(&info.dst.u3.ip6, (void *)&inet->pinet6->daddr,
sizeof(info.dst.u3.ip6));
/* check tickets */
- if ((verdict = snet_ticket_check(&info)) != SNET_VERDICT_NONE)
+ verdict = snet_ticket_check(&info);
+ if (verdict != SNET_VERDICT_NONE)
goto out;
/* inserting verdict PENDING */
info.verdict_id = snet_verdict_insert();
--
1.6.3.3
^ permalink raw reply related
* Re: [RFC v2 00/10] snet: Security for NETwork syscalls
From: Samir Bellabes @ 2010-03-06 18:17 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-kernel, netdev, netfilter-devel, hadi, kaber, zbr, nhorman,
root, linux-security-module
In-Reply-To: <201003030156.o231udx1023055@www262.sakura.ne.jp>
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
> Regarding [RFC v2 05/10] snet: introduce snet_event
> +static rwlock_t snet_evh_lock = __RW_LOCK_UNLOCKED();
>
> You can use "static DEFINE_RWLOCK(snet_evh_lock);".
>
> +int snet_event_is_registered(const enum snet_syscall syscall, const u8 protocol)
>
> Maybe rcu_read_lock() is better than rw spinlock because this function is
> frequently called.
here is the patch.
thank you Tetsuo
>From 34715d4d900aee2d3759e3008313b65dc2f130fd Mon Sep 17 00:00:00 2001
From: Samir Bellabes <sam@synack.fr>
Date: Sat, 6 Mar 2010 15:44:42 +0100
Subject: [PATCH 1/3] snet: use proper rwlock_t declaration
Noticed by Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Samir Bellabes <sam@synack.fr>
---
security/snet/snet_event.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/security/snet/snet_event.c b/security/snet/snet_event.c
index 9e3f7d2..5f708d0 100644
--- a/security/snet/snet_event.c
+++ b/security/snet/snet_event.c
@@ -9,7 +9,7 @@
#include "snet_utils.h"
static struct list_head *snet_evh;
-static rwlock_t snet_evh_lock = __RW_LOCK_UNLOCKED();
+static DEFINE_RWLOCK(snet_evh_lock);
struct snet_event_entry {
struct list_head list;
--
1.6.3.3
^ permalink raw reply related
* Re: [RFC v2 00/10] snet: Security for NETwork syscalls
From: Samir Bellabes @ 2010-03-06 18:16 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-kernel, netdev, netfilter-devel, hadi, kaber, zbr, nhorman,
root, linux-security-module
In-Reply-To: <201003030156.o231udx1023055@www262.sakura.ne.jp>
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
> Regarding [RFC v2 04/10] snet: introduce snet_core
> +static __init int snet_init(void)
> +{
> + int ret;
> +
> + pr_debug("initializing: event_hash_size=%u "
> + "verdict_hash_size=%u verdict_delay=%usecs "
> + "default_policy=%s\n",
> + snet_evh_size, snet_vdh_size, snet_verdict_delay,
> + snet_verdict_name(snet_verdict_policy));
>
> Why not to stop here if snet_evh_size == 0 or snet_vdh_size == 0 in order to
> avoid "division by 0".
indeed. I applied this patch
>From 593614c92a1f2058c014fa674c67f434b24b26e4 Mon Sep 17 00:00:00 2001
From: Samir Bellabes <sam@synack.fr>
Date: Sat, 6 Mar 2010 17:32:51 +0100
Subject: [PATCH 2/3] snet: adding checks for bad configuration values
this patch adds some checks on boot parameters and runtime configurations for:
- snet_verdict_policy, snet_verdict_delay and snet_vdh_size
- snet_evh_size
- snet_ticket_delay and snet_ticket_mode
Noticed by Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Samir Bellabes <sam@synack.fr>
---
include/linux/snet.h | 1 +
security/snet/snet_core.c | 6 ++++++
security/snet/snet_event.c | 6 ++++++
security/snet/snet_netlink.c | 27 ++++++++++++++++++++-------
security/snet/snet_ticket.c | 12 ++++++++++++
security/snet/snet_verdict.c | 12 ++++++++++++
6 files changed, 57 insertions(+), 7 deletions(-)
diff --git a/include/linux/snet.h b/include/linux/snet.h
index 739601d..e6e2d52 100644
--- a/include/linux/snet.h
+++ b/include/linux/snet.h
@@ -41,6 +41,7 @@ enum snet_ticket_mode {
SNET_TICKET_OFF = 0,
SNET_TICKET_FIX,
SNET_TICKET_EXTEND,
+ SNET_TICKET_INVALID,
};
/* genetlink commands */
diff --git a/security/snet/snet_core.c b/security/snet/snet_core.c
index 9f2eb2e..949ecaa 100644
--- a/security/snet/snet_core.c
+++ b/security/snet/snet_core.c
@@ -42,6 +42,12 @@ static __init int snet_init(void)
snet_evh_size, snet_vdh_size, snet_verdict_delay,
snet_verdict_name(snet_verdict_policy));
+ if (snet_verdict_policy >= SNET_VERDICT_INVALID) {
+ printk(KERN_ERR "snet: bad snet_verdict_policy\n");
+ ret = -EINVAL;
+ goto event_failed;
+ }
+
ret = snet_event_init();
if (ret < 0)
goto event_failed;
diff --git a/security/snet/snet_event.c b/security/snet/snet_event.c
index 5f708d0..5693aac 100644
--- a/security/snet/snet_event.c
+++ b/security/snet/snet_event.c
@@ -165,6 +165,12 @@ int snet_event_init(void)
{
int err = 0, i = 0;
+ if (snet_evh_size == 0) {
+ printk(KERN_ERR "snet: bad snet_evh_size value\n");
+ err = -EINVAL;
+ goto out;
+ }
+
snet_evh = kzalloc(sizeof(struct list_head) * snet_evh_size,
GFP_KERNEL);
if (!snet_evh) {
diff --git a/security/snet/snet_netlink.c b/security/snet/snet_netlink.c
index b0dd163..937b0fc 100644
--- a/security/snet/snet_netlink.c
+++ b/security/snet/snet_netlink.c
@@ -363,25 +363,38 @@ out:
static int snet_nl_config(struct sk_buff *skb,
struct genl_info *info)
{
- int ret = -EINVAL;
+ int ret = 0;
atomic_set(&snet_nl_seq, info->snd_seq);
if (info->attrs[SNET_A_VERDICT_DELAY]) {
- snet_verdict_delay = nla_get_u32(info->attrs[SNET_A_VERDICT_DELAY]);
+ unsigned int new = nla_get_u32(info->attrs[SNET_A_VERDICT_DELAY]);
+ if (new == 0) {
+ ret = -EINVAL;
+ goto out;
+ }
+ snet_verdict_delay = new;
pr_debug("snet_nl_config: verdict_delay=%u\n", snet_verdict_delay);
- ret = 0;
}
if (info->attrs[SNET_A_TICKET_DELAY]) {
- snet_ticket_delay = nla_get_u32(info->attrs[SNET_A_TICKET_DELAY]);
+ unsigned int new = nla_get_u32(info->attrs[SNET_A_TICKET_DELAY]);
+ if (new == 0) {
+ ret = -EINVAL;
+ goto out;
+ }
+ snet_ticket_delay = new;
pr_debug("snet_nl_config: ticket_delay=%u\n", snet_ticket_delay);
- ret = 0;
}
if (info->attrs[SNET_A_TICKET_MODE]) {
- snet_ticket_mode = nla_get_u32(info->attrs[SNET_A_TICKET_MODE]);
+ unsigned int new = nla_get_u32(info->attrs[SNET_A_TICKET_MODE]);
+ if (new >= SNET_TICKET_INVALID) {
+ ret = -EINVAL;
+ goto out;
+ }
+ snet_ticket_mode = new;
pr_debug("snet_nl_config: ticket_mode=%u\n", snet_ticket_mode);
- ret = 0;
}
+out:
return ret;
}
diff --git a/security/snet/snet_ticket.c b/security/snet/snet_ticket.c
index 62ced7b..80a1b0f 100644
--- a/security/snet/snet_ticket.c
+++ b/security/snet/snet_ticket.c
@@ -158,6 +158,18 @@ int snet_ticket_init(void)
struct cred *cred = (struct cred *) current->real_cred;
struct snet_task_security *tsec = NULL;
+ if (snet_ticket_mode >= SNET_TICKET_INVALID) {
+ printk(KERN_ERR "snet: bad snet_ticket_mode\n");
+ return -EINVAL;
+ }
+
+ if ((snet_ticket_mode == SNET_TICKET_FIX ||
+ snet_ticket_mode == SNET_TICKET_EXTEND) &&
+ (snet_ticket_delay == 0)) {
+ printk(KERN_ERR "snet: bad snet_ticket_delay\n");
+ return -EINVAL;
+ }
+
tsec = kzalloc(sizeof(struct snet_task_security), GFP_KERNEL);
if (tsec == NULL)
return -ENOMEM;
diff --git a/security/snet/snet_verdict.c b/security/snet/snet_verdict.c
index 480a7f8..ba35d19 100644
--- a/security/snet/snet_verdict.c
+++ b/security/snet/snet_verdict.c
@@ -156,6 +156,18 @@ int snet_verdict_init(void)
{
int err = 0, i = 0;
+ if (snet_vdh_size == 0) {
+ printk(KERN_ERR "snet: bad snet_vdh_size value\n");
+ err = -EINVAL;
+ goto out;
+ }
+
+ if (snet_verdict_delay == 0) {
+ printk(KERN_ERR "snet: bad snet_verdict_delay value\n");
+ err = -EINVAL;
+ goto out;
+ }
+
snet_vdh = kzalloc(sizeof(struct list_head) * snet_vdh_size,
GFP_KERNEL);
if (!snet_vdh) {
--
1.6.3.3
^ permalink raw reply related
* Re: [patch] bluetooth: debugfs changes use too much stack
From: Marcel Holtmann @ 2010-03-06 17:49 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jan Ceuleers, David S. Miller, Dave Young, Greg Kroah-Hartman,
Roger Quadros, Bing Zhao, linux-bluetooth, netdev,
kernel-janitors
In-Reply-To: <20100306153030.GU4958@bicker>
Hi Dan,
> > Error handling?
> >
> > > The original code would break with a 4K stack.
> > >
> > > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > > ---
> > > This was compile tested only. Sorry about that.
> > >
> > > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > > index 1a79a6c..835758f 100644
> > > --- a/net/bluetooth/hci_sysfs.c
> > > +++ b/net/bluetooth/hci_sysfs.c
> > > @@ -417,9 +417,11 @@ static ssize_t inquiry_cache_read(struct file *file, char __user *userbuf,
> > > struct hci_dev *hdev = file->private_data;
> > > struct inquiry_cache *cache = &hdev->inq_cache;
> > > struct inquiry_entry *e;
> > > - char buf[4096];
> > > + char *buf;
> > > int n = 0;
> > > + ssize_t ret;
> > >
> > > + buf = kmalloc(4096, GFP_KERNEL);
> >
> > Could this kmalloc not fail?
>
> Grr... I'm really sorry about that.
>
> I will send an updated patch tomorrow.
please don't since we fixed this already in the net-2.6 tree.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] MAINTAINER: Correct CAN Maintainer responsibilities and paths
From: Wolfgang Grandegger @ 2010-03-06 17:48 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: SocketCAN Core Mailing List, Linux Netdev List, Urs Thuermann,
David Miller, Oliver Hartkopp
In-Reply-To: <4B929F96.1030402-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
Oliver Hartkopp wrote:
> Update the CAN Maintainer responsibilities and add source paths.
> Additional the SocketCAN core ML is not subscribers-only anymore.
>
> Signed-off-by: Oliver Hartkopp <socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
>
> ---
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 34f52a1..9ee1f78 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1372,20 +1372,28 @@ F: arch/x86/include/asm/calgary.h
> F: arch/x86/include/asm/tce.h
>
> CAN NETWORK LAYER
> -M: Urs Thuermann <urs.thuermann-l29pVbxQd1IUtdQbppsyvg@public.gmane.org>
> +M: Oliver Hartkopp <socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
> M: Oliver Hartkopp <oliver.hartkopp-l29pVbxQd1IUtdQbppsyvg@public.gmane.org>
> -L: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org (subscribers-only)
> +M: Urs Thuermann <urs.thuermann-l29pVbxQd1IUtdQbppsyvg@public.gmane.org>
> +L: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> W: http://developer.berlios.de/projects/socketcan/
> S: Maintained
> -F: drivers/net/can/
> -F: include/linux/can/
> +F: net/can/
> F: include/linux/can.h
> +F: include/linux/can/core.h
> +F: include/linux/can/bcm.h
> +F: include/linux/can/raw.h
>
> CAN NETWORK DRIVERS
> M: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
> -L: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org (subscribers-only)
> +L: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> W: http://developer.berlios.de/projects/socketcan/
> S: Maintained
> +F: drivers/net/can/
> +F: include/linux/can/dev.h
> +F: include/linux/can/error.h
> +F: include/linux/can/netlink.h
> +F: include/linux/can/platform/
>
> CELL BROADBAND ENGINE ARCHITECTURE
> M: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>
Looks good ==>
Acked-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
^ 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