Re: [PATCH] [1/2] kernel error reporting (revised)

[Jim Keniston <jkenisto@us.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2323 bytes --]

Andrew Morton wrote:
> 
> Jim Keniston <jkenisto@us.ibm.com> wrote:
> >
> > +int kernel_error_event_iov(const struct iovec *iov, unsigned int nseg,
> > +     u32 groups)
> > +{
> > ...
> > +
> > +     return netlink_broadcast(kerror_nl, skb, 0, ~0, GFP_ATOMIC);
> 
> 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.
-----

[-- Attachment #2: kerror.patch.txt --]
[-- Type: text/plain, Size: 1984 bytes --]

--- kerror.c.old	Thu Jul 17 10:53:19 2003
+++ kerror.c	Thu Jul 17 10:54:55 2003
@@ -3,10 +3,12 @@
  * Copyright (C) 2003 David S. Miller (davem@redhat.com)
  * June 2003 - Jim Keniston and Dan Stekloff (kenistoj and dsteklof@us.ibm.com)
  *	Fixed a couple of bugs and added iovec interface.
+ * July 2003 - Jim Keniston - Added handling of packets logged from IRQ context.
  */
 
 #include <linux/kernel.h>
 #include <linux/types.h>
+#include <linux/interrupt.h>
 #include <linux/skbuff.h>
 #include <linux/socket.h>
 #include <linux/netlink.h>
@@ -17,6 +19,33 @@
 
 static struct sock *kerror_nl;
 
+/* Packets logged from IRQ context are queued for broadcast by a tasklet. */
+static struct sk_buff_head delayed_pkts;
+static void broadcast_delayed_pkts(unsigned long);
+static DECLARE_TASKLET(delayed_pkts_tasklet, broadcast_delayed_pkts, 0);
+
+/**
+ * delayed_broadcast() - Schedule a tasklet to broadcast a packet.
+ * We want to broadcast the indicated packet, but can't because we're
+ * in a hardware interrupt and so can't call netlink_broadcast().
+ * Schedule a tasklet to do the job.
+ *
+ * @skb: the socket buffer to broadcast
+ */
+static void delayed_broadcast(struct sk_buff *skb)
+{
+	skb_queue_tail(&delayed_pkts, skb);
+	tasklet_schedule(&delayed_pkts_tasklet);
+}
+
+static void broadcast_delayed_pkts(unsigned long ignored)
+{
+	struct sk_buff *skb;
+	while ((skb = skb_dequeue(&delayed_pkts)) != NULL) {
+		(void) netlink_broadcast(kerror_nl, skb, 0, ~0, GFP_ATOMIC);
+	}
+}
+
 /**
  * kernel_error_event_iov() - Broadcast packet to NETLINK_KERROR sockets.
  * @iov: the packet's data
@@ -54,6 +83,11 @@
 
 	NETLINK_CB(skb).dst_groups = groups;
 
+	if (in_irq()) {
+		delayed_broadcast(skb);
+		return -EINPROGRESS;
+	}
+
 	return netlink_broadcast(kerror_nl, skb, 0, ~0, GFP_ATOMIC);
 
 nlmsg_failure:
@@ -85,6 +119,7 @@
 	if (kerror_nl == NULL)
 		panic("kerror_init: cannot initialize kerror_nl\n");
 
+	skb_queue_head_init(&delayed_pkts);
 	return 0;
 }