From: James Morris <hidden> Date: 2003-07-10 04:28:20
On Tue, 8 Jul 2003, Jim Keniston wrote:
+ kerror_nl = netlink_kernel_create(NETLINK_KERROR, kerror_netlink_rcv);
+ if (kerror_nl == NULL)
+ panic("kerror_init: cannot initialize kerror_nl\n");
You can simply use NULL instead of passing the dummy kerror_netlink_rcv
function.
+struct kern_log_entry {
+ __u16 log_kmagic; /* always LOGREC_KMAGIC */
+ __u16 log_kversion; /* which version of this struct? */
+ char log_facility[FACILITY_MAXLEN]; /* e.g., driver name */
These fields should generally be specified in ascending order to help with
alignment.
It may also be worth looking at how the ULOG code batches messages to
improve peformance.
- James
--
James Morris
[off-list ref]
From: Jim Keniston <hidden> Date: 2003-07-10 18:58:38
James Morris wrote:
On Tue, 8 Jul 2003, Jim Keniston wrote:
+ kerror_nl = netlink_kernel_create(NETLINK_KERROR, kerror_netlink_rcv);
+ if (kerror_nl == NULL)
+ panic("kerror_init: cannot initialize kerror_nl\n");
You can simply use NULL instead of passing the dummy kerror_netlink_rcv
function.
That begs the question: do we trust that nobody but the kernel will send
packets to a NETLINK_KERROR socket? Ordinary users can't, but any root
application can. Without kerror_netlink_rcv(), such packets don't get
dequeued.
+struct kern_log_entry {
+ __u16 log_kmagic; /* always LOGREC_KMAGIC */
+ __u16 log_kversion; /* which version of this struct? */
+ char log_facility[FACILITY_MAXLEN]; /* e.g., driver name */
These fields should generally be specified in ascending order to help with
alignment.
We include log_kmagic and log_kversion so the receiving app (e.g. the evlog
daemon) can figure out which version of the kernel header it's getting. Note
that we're up to #3 (going on #4, with the changes you and others have
suggested). As long as we have to include log_kmagic and log_kversion, they
need to be first.
That said, I get the impression that people would be more comfortable if
log_facility[] were moved to the end. Other than that, can anybody point
out a specific area where there's likely to be an alignment problem? The
various members' offsets are the same on i386, ppc32, and ppc64. This should
also be true for s390 and s390x. I'd think it'd really matter only when kernel
and user on the same system are different architectures. ppc64K/ppc32U,
at least, works.
It may also be worth looking at how the ULOG code batches messages to
improve peformance.
Thanks for the pointer. Looks nice. If performance turns out to be an issue
(e.g., if for some reason people want to cc all printk messages through this
interface), I'll keep that in mind.
From: James Morris <hidden> Date: 2003-07-11 15:23:21
On Thu, 10 Jul 2003, Jim Keniston wrote:
James Morris wrote:
quoted
On Tue, 8 Jul 2003, Jim Keniston wrote:
+ kerror_nl = netlink_kernel_create(NETLINK_KERROR, kerror_netlink_rcv);
+ if (kerror_nl == NULL)
+ panic("kerror_init: cannot initialize kerror_nl\n");
You can simply use NULL instead of passing the dummy kerror_netlink_rcv
function.
That begs the question: do we trust that nobody but the kernel will send
packets to a NETLINK_KERROR socket? Ordinary users can't, but any root
application can. Without kerror_netlink_rcv(), such packets don't get
dequeued.
Indeed, the kernel socket buffer fills up.
I think this needs to be addressed in the netlink code, per the patch
below.
Comments?
- James
--
James Morris
[off-list ref]
diff -NurX dontdiff linux-2.5.75.orig/net/netlink/af_netlink.c linux-2.5.75.w1/net/netlink/af_netlink.c
@@ -430,6 +430,10 @@gotono_dst;nlk=nlk_sk(sk);+/* Don't bother queuing skb if kernel socket has no input function */+if(nlk->pid==0&&!nlk->data_ready)+gotono_dst;+#ifdef NL_EMULATE_DEVif(nlk->handler){skb_orphan(skb);
From: David S. Miller <hidden> Date: 2003-07-12 05:04:09
On Sat, 12 Jul 2003 01:37:44 +1000 (EST)
James Morris [off-list ref] wrote:
On Thu, 10 Jul 2003, Jim Keniston wrote:
quoted
That begs the question: do we trust that nobody but the kernel will send
packets to a NETLINK_KERROR socket? Ordinary users can't, but any root
application can. Without kerror_netlink_rcv(), such packets don't get
dequeued.
Indeed, the kernel socket buffer fills up.
I think this needs to be addressed in the netlink code, per the patch
below.
From: David S. Miller <hidden> Date: 2003-07-12 05:35:59
On Sat, 12 Jul 2003 01:37:44 +1000 (EST)
James Morris [off-list ref] wrote:
Indeed, the kernel socket buffer fills up.
I think this needs to be addressed in the netlink code, per the patch
below.
...
+ /* Don't bother queuing skb if kernel socket has no input function */
+ if (nlk->pid == 0 && !nlk->data_ready)
+ goto no_dst;
+
Oops, turns out this doesn't work. data_ready is never NULL, look at
how netlink_kernel_create() works.
Also, the broadcast case probably needs to be handled
too?
As an aside, to be honest what's so wrong with the socket receive
buffer filling up? The damage is limited to the receive buffer size
of the kernel netlink socket, but that's it.
From: James Morris <hidden> Date: 2003-07-13 01:03:11
On Fri, 11 Jul 2003, David S. Miller wrote:
quoted
+ /* Don't bother queuing skb if kernel socket has no input function */
+ if (nlk->pid == 0 && !nlk->data_ready)
+ goto no_dst;
+
Oops, turns out this doesn't work. data_ready is never NULL, look at
how netlink_kernel_create() works.
It's ok: sk->data_ready is never null, but nlk_sk(sk)->data_ready will be
null unless an input function is provided there.
Also, the broadcast case probably needs to be handled
too?
Netlink sockets created by netlink_kernel_create() do not subscribe to any
groups and are not broadcast to.
As an aside, to be honest what's so wrong with the socket receive
buffer filling up? The damage is limited to the receive buffer size
of the kernel netlink socket, but that's it.
Agreed, it's not really harmful, but it's sloppy. Better to let the
application know that it can't send to the socket rather than let it keep
sending (with successful return codes) until the kernel socket buffer
fills up.
- James
--
James Morris
[off-list ref]
From: Jim Keniston <hidden> Date: 2003-07-15 17:31:35
Jim Keniston [off-list ref] wrote:
Subject: [PATCH - RFC] [1/2] 2.6 must-fix list - kernel error reporting
Andrew Morton's 2.6 must-fix list includes the following item:
quoted
o We need a kernel side API for reporting error events to userspace (could
be async to 2.6 itself)
(Prototype core based on netlink exists)
The enclosed patches provide a mechanism for reporting error events
to user-mode applications via netlink. This mechanism supplements
the text-oriented printk mechanism, providing a way to log binary
data or a mixture of text+binary.
...
Here are updated patches, reflecting the following changes:
Patch #1 (kerror.c et al):
- Given James Morris's patch to af_netlink.c (Rev 1.30 in BitKeeper), I
was able to remove kerror_netlink_rcv(). (My patches work fine without
this, except that any packets sent to the NETLINK_KERROR socket by an
ill-behaved, root-owned application would accumulate in the kernel's
socket buffer.)
Patch #2 (evlog.c et al -- see accompanying post):
- Paraphrase dropped packets via printk() when nobody's listening to
netlink socket.
- Added support for 'z' qualifier, to resync with vsnprintf().
- In evlog.h, reordered members of struct kern_log_entry to address
alignment worries.
These patches work for both 2.5.74 and 2.5.75.
Jim Keniston
IBM Linux Technology Center
http://prdownloads.sourceforge.net/evlog/kerror-2.5.75.patch?downloadhttp://prdownloads.sourceforge.net/evlog/evlog-2.5.75.patch?downloadhttp://prdownloads.sourceforge.net/evlog/kerrord.tar.gz?download
This appears to be deadlocky when called from interrupt handlers.
netlink_broadcast() does read_lock(&nl_table_lock). But nl_table_lock is
not an irq-safe lock.
Possibly netlink_broadcast() can be made callable from hardirq context, but
it looks to be non trivial. The various error and delivery handlers need
to be reviewed, the kfree_skb() calls should be thought about, etc.
This appears to be deadlocky when called from interrupt handlers.
netlink_broadcast() does read_lock(&nl_table_lock). But nl_table_lock is
not an irq-safe lock.
Possibly netlink_broadcast() can be made callable from hardirq context, but
it looks to be non trivial. The various error and delivery handlers need
to be reviewed,
Yes indeed. I believe this issue is resolved by detecting that we're in an
interrupt handler and delaying the call to netlink_broadcast() via a tasklet.
See the enclosed patch to the previously posted rev. I'll update the full patch at
http://prdownloads.sourceforge.net/evlog/kerror-2.5.75.patch?download
An issue remains: what, if anything, to tell the caller if the delayed
netlink_broadcast() fails. See below for further thoughts.
the kfree_skb() calls should be thought about, etc.
I've thought about them. :-) Given the aforementioned solution, I don't think
kfree_skb() is an issue, because it's called as needed by netlink_broadcast().
If I'm missing something here, feel free to clarify.
Thanks.
Jim
WHAT IF THE DELAYED netlink_broadcast() CALL FAILS?
1. Can we detect from IRQ context that nobody is listening, and thereby
return -ESRCH to the caller? No, to do that would require perusing the
nl_table[NETLINK_KERROR] list. We can't do that for the same reason we
can't call netlink_broadcast(). So kernel_error_event_iov() now returns
-EINPROGRESS if it had to delay the netlink_broadcast() call.
2. Could the tasklet report netlink_broadcast() failures back to the
higher-level code? Yes, we could implement a per-group callback to handle
that. Current thinking is that it's overkill. But it would resolve
the next issue...
3. Given the above, what should the evlog.c caller do when
kernel_error_event_iov() returns -EINPROGRESS?
a. Nothing. Figure the packet will probably get logged.
b. Just to be safe, report it via printk, the same way we report dropped
packets.
We currently do (a). (b) would mean that every event logged from IRQ
context would be cc-ed to printk.
-----
From: James Morris <hidden> Date: 2003-07-18 01:38:35
On Thu, 17 Jul 2003, Jim Keniston wrote:
3. Given the above, what should the evlog.c caller do when
kernel_error_event_iov() returns -EINPROGRESS?
a. Nothing. Figure the packet will probably get logged.
b. Just to be safe, report it via printk, the same way we report dropped
packets.
We currently do (a). (b) would mean that every event logged from IRQ
context would be cc-ed to printk.
I don't think this irq detection logic should be added at all here, let
the caller reschedule its logging if running in irq context.
- James
--
James Morris
[off-list ref]
From: Jim Keniston <hidden> Date: 2003-07-18 16:55:32
James Morris wrote:
On Thu, 17 Jul 2003, Jim Keniston wrote:
quoted
3. Given the above, what should the evlog.c caller do when
kernel_error_event_iov() returns -EINPROGRESS?
a. Nothing. Figure the packet will probably get logged.
b. Just to be safe, report it via printk, the same way we report dropped
packets.
We currently do (a). (b) would mean that every event logged from IRQ
context would be cc-ed to printk.
I don't think this irq detection logic should be added at all here, let
the caller reschedule its logging if running in irq context.
- James
--
James Morris
[off-list ref]
Yes, this makes sense. At the kerror.c level, just return -EDEADLK if in_irq().
Delay packet delivery (via a tasklet, as before) at the evlog.c level instead.
That way, we know at the evlog.c level (in the tasklet) whether the event packet
was delivered to anybody, and can paraphrase it to printk if it wasn't.
Is this the sort of thing you had in mind?
Jim K
From: Jim Keniston <hidden> Date: 2003-07-18 23:17:49
Jim Keniston wrote:
James Morris wrote:
quoted
On Thu, 17 Jul 2003, Jim Keniston wrote:
quoted
3. Given the above, what should the evlog.c caller do when
kernel_error_event_iov() returns -EINPROGRESS?
a. Nothing. Figure the packet will probably get logged.
b. Just to be safe, report it via printk, the same way we report dropped
packets.
We currently do (a). (b) would mean that every event logged from IRQ
context would be cc-ed to printk.
I don't think this irq detection logic should be added at all here, let
the caller reschedule its logging if running in irq context.
- James
--
James Morris
[off-list ref]
Yes, this makes sense. At the kerror.c level, just return -EDEADLK if in_irq().
Delay packet delivery (via a tasklet, as before) at the evlog.c level instead.
That way, we know at the evlog.c level (in the tasklet) whether the event packet
was delivered to anybody, and can paraphrase it to printk if it wasn't.
Is this the sort of thing you had in mind?
Jim K
From: James Morris <hidden> Date: 2003-07-19 23:37:59
On Fri, 18 Jul 2003, Jim Keniston wrote:
quoted
Yes, this makes sense. At the kerror.c level, just return -EDEADLK if in_irq().
Delay packet delivery (via a tasklet, as before) at the evlog.c level instead.
That way, we know at the evlog.c level (in the tasklet) whether the event packet
was delivered to anybody, and can paraphrase it to printk if it wasn't.
Is this the sort of thing you had in mind?
Not exactly -- I don't think the logging framework should do any irq
detection. The caller should either know if its in an interrupt, or do
the detection itself.
- James
--
James Morris
[off-list ref]