Re: [PATCH] 2.2.13 ip_fw.c interrupt and linecount fixes (was Re: repeatedly reading ipchains rules

Juanjo Ciarlante (jjo@mendoza.gov.ar)
Thu, 21 Oct 1999 14:00:59 -0300


On Thu, Oct 21, 1999 at 01:04:35PM +1000, Paul Rusty Russell wrote:
> OK, this is what I'm going to submit to Alan for 2.2.14; please report
> any problems (I don't expect any).
> {patch snipped}

>
> --- linux/net/ipv4/ip_fw.c.~1~ Thu Oct 21 10:33:36 1999
> +++ linux/net/ipv4/ip_fw.c Thu Oct 21 13:00:16 1999
> @@ -40,11 +40,13 @@
> * 23-Jul-1999: Fixed small fragment security exposure opened on 15-May-1998.
> * John McDonald <jm@dataprotect.com>
> * Thomas Lopatic <tl@dataprotect.com>
> + * 21-Oct-1999: Use bh, not interrupt locking. --RR
> + * Applied count fix by Emanuele Caratti <wiz@iol.it>
> */
>
> /*
> *
> - * The origina Linux port was done Alan Cox, with changes/fixes from
> + * The original Linux port was done Alan Cox, with changes/fixes from
> * Pauline Middlelink, Jos Vos, Thomas Quinot, Wouter Gadeyne, Juan
> * Jose Ciarlante, Bernd Eckenfels, Keith Owens and others.
> *
> @@ -150,6 +152,7 @@
> static struct sock *ipfwsk;
> #endif
>
> +/* Don't call SLOT_NUMBER when you have a write lock. */
> #ifdef __SMP__
> #define SLOT_NUMBER() (cpu_number_map[smp_processor_id()]*2 + !in_interrupt())
> #else
> @@ -190,21 +193,26 @@
> __FILE__, __LINE__, SLOT_NUMBER()); \
> } while (0)
>
> +#define FWC_NOINT() \
> +do { \
> + if (in_interrupt()) \
> + printk("Rusty, you promised! %s %u\n", __FILE__, __LINE__); \
> +} while(0)
> #else
> #define FWC_DEBUG_LOCK(d) do { } while(0)
> #define FWC_DEBUG_UNLOCK(d) do { } while(0)
> #define FWC_DONT_HAVE_LOCK(d) do { } while(0)
> #define FWC_HAVE_LOCK(d) do { } while(0)
> +#define FWC_NOINT() do { } while(0)
> #endif /*DEBUG_IP_FIRWALL_LOCKING*/
>
> +/* We never to a write lock in bh, so we only need write_lock_bh */
> #define FWC_READ_LOCK(l) do { FWC_DEBUG_LOCK(fwc_rlocks); read_lock(l); } while (0)
> -#define FWC_WRITE_LOCK(l) do { FWC_DEBUG_LOCK(fwc_wlocks); write_lock(l); } while (0)
> -#define FWC_READ_LOCK_IRQ(l,f) do { FWC_DEBUG_LOCK(fwc_rlocks); read_lock_irqsave(l,f); } while (0)
> -#define FWC_WRITE_LOCK_IRQ(l,f) do { FWC_DEBUG_LOCK(fwc_wlocks); write_lock_irqsave(l,f); } while (0)
> +/* Debug after lock obtained, (in_interrupt() will be true there), so
> + SLOT_NUMBER consistent. */
> +#define FWC_WRITE_LOCK(l) do { FWC_NOINT(); write_lock_bh(l); FWC_DEBUG_LOCK(fwc_wlocks); } while (0)
> #define FWC_READ_UNLOCK(l) do { FWC_DEBUG_UNLOCK(fwc_rlocks); read_unlock(l); } while (0)
> -#define FWC_WRITE_UNLOCK(l) do { FWC_DEBUG_UNLOCK(fwc_wlocks); write_unlock(l); } while (0)
> -#define FWC_READ_UNLOCK_IRQ(l,f) do { FWC_DEBUG_UNLOCK(fwc_rlocks); read_unlock_irqrestore(l,f); } while (0)
> -#define FWC_WRITE_UNLOCK_IRQ(l,f) do { FWC_DEBUG_UNLOCK(fwc_wlocks); write_unlock_irqrestore(l,f); } while (0)
> +#define FWC_WRITE_UNLOCK(l) do { FWC_DEBUG_UNLOCK(fwc_wlocks); write_unlock_bh(l); } while (0)
>
> struct ip_chain;
>
> @@ -228,6 +236,7 @@
> {
> struct ip_chain *prevchain; /* Pointer to referencing chain */
> struct ip_fwkernel *prevrule; /* Pointer to referencing rule */
> + unsigned int count;
> struct ip_counters counters;
> };
>
> @@ -729,8 +738,8 @@
> else FWC_HAVE_LOCK(fwc_rlocks);
>
> f = chain->chain;
> + count = 0;
> do {
> - count = 0;
> for (; f; f = f->next) {
> count++;
> if (ip_rule_match(f,rif,ip,
> @@ -768,10 +777,12 @@
> else {
> f->branch->reent[slot].prevchain
> = chain;
> + f->branch->reent[slot].count = count;
> f->branch->reent[slot].prevrule
> = f->next;
> chain = f->branch;
> f = chain->chain;
> + count = 0;
> }
> }
> else if (f->simplebranch == FW_SKIP)
> @@ -790,6 +801,7 @@
> if (chain->reent[slot].prevchain) {
> struct ip_chain *tmp = chain;
> f = chain->reent[slot].prevrule;
> + count = chain->reent[slot].count;
> chain = chain->reent[slot].prevchain;
> tmp->reent[slot].prevchain = NULL;
> }
> @@ -1300,9 +1312,8 @@
> {
> int ret;
> struct ip_chain *chain;
> - unsigned long flags;
>
> - FWC_WRITE_LOCK_IRQ(&ip_fw_lock, flags);
> + FWC_WRITE_LOCK(&ip_fw_lock);
>
> switch (cmd) {
> case IP_FW_FLUSH:
> @@ -1326,7 +1337,7 @@
> struct iphdr *ip;
>
> /* Don't need write lock. */
> - FWC_WRITE_UNLOCK_IRQ(&ip_fw_lock, flags);
> + FWC_WRITE_UNLOCK(&ip_fw_lock);
>
> if (len != sizeof(struct ip_fwtest) || !check_label(m))
> return EINVAL;
> @@ -1521,7 +1532,7 @@
> ret = EINVAL;
> }
>
> - FWC_WRITE_UNLOCK_IRQ(&ip_fw_lock, flags);
> + FWC_WRITE_UNLOCK(&ip_fw_lock);
> return ret;
> }
>
> @@ -1582,7 +1593,6 @@
> {
> struct ip_chain *i;
> struct ip_fwkernel *j = ip_fw_chains->chain;
> - unsigned long flags;
> int len = 0;
> int last_len = 0;
> off_t upto = 0;
> @@ -1591,7 +1601,7 @@
> duprintf("ip_fw_chains is 0x%0lX\n", (unsigned long int)ip_fw_chains);
>
> /* Need a write lock to lock out ``readers'' which update counters. */
> - FWC_WRITE_LOCK_IRQ(&ip_fw_lock, flags);
> + FWC_WRITE_LOCK(&ip_fw_lock);
>
> for (i = ip_fw_chains; i; i = i->next) {
> for (j = i->chain; j; j = j->next) {
> @@ -1622,7 +1632,7 @@
> }
> }
> outside:
> - FWC_WRITE_UNLOCK_IRQ(&ip_fw_lock, flags);
> + FWC_WRITE_UNLOCK(&ip_fw_lock);
> buffer[len] = '\0';
>
> duprintf("ip_chain_procinfo: Length = %i (of %i). Offset = %li.\n",
> @@ -1638,10 +1648,9 @@
> struct ip_chain *i;
> int len = 0,last_len = 0;
> off_t pos = 0,begin = 0;
> - unsigned long flags;
>
> /* Need a write lock to lock out ``readers'' which update counters. */
> - FWC_WRITE_LOCK_IRQ(&ip_fw_lock, flags);
> + FWC_WRITE_LOCK(&ip_fw_lock);
>
> for (i = ip_fw_chains; i; i = i->next)
> {
> @@ -1673,7 +1682,7 @@
>
> last_len = len;
> }
> - FWC_WRITE_UNLOCK_IRQ(&ip_fw_lock, flags);
> + FWC_WRITE_UNLOCK(&ip_fw_lock);
>
> *start = buffer+(offset-begin);
> len-=(offset-begin);
> --
> Hacking time.
Cool!.. (as a bh-beated code, fw code wanted this)

Just a litl warning: xxxx_lock_bh() macros are in net/ip_masq.h,
I put them under ip_masq when I created these beasts (in fact, in 2.2
ip_masq*.c are the ONLY callers) because I did not have enough knowlegde
(coward, Me?) to alter spinlock.h, so
problems may arise because of the #ifdef CONFIG_IP_MASQUERADE when
include'ing ip_masq.h in ip_fw.c.

In short, xxxx_lock_bh() macros should be ``blessed'' with higher visibility.

Juanjo

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/