From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Graf Subject: Re: BK-current build breakage Date: Fri, 7 Jan 2005 01:10:53 +0100 Message-ID: <20050107001053.GY26856@postel.suug.ch> References: <41DDCF00.6070405@pobox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "David S. Miller" , Netdev Return-path: To: Jeff Garzik Content-Disposition: inline In-Reply-To: <41DDCF00.6070405@pobox.com> Sender: netdev-bounce@oss.sgi.com Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org * Jeff Garzik <41DDCF00.6070405@pobox.com> 2005-01-06 18:51 > net/built-in.o(.text+0x25858): In function `tcf_ipt_init': > net/sched/ipt.c:63: undefined reference to `__ipt_find_target_lock' > net/built-in.o(.text+0x25873):net/sched/ipt.c:78: undefined reference to > `__ipt_mutex_up' > make: *** [.tmp_vmlinux1] Error 1 A fix for this is in the queue: On Wed, 2005-01-05 at 19:12 +0100, Thomas Graf wrote: > * Rusty Russell <1104896029.20582.72.camel@localhost.localdomain> 2005-01-05 14:33 > > Name: Clean up the kmod handling code in iptables.c > > Status: Tested under nfsim > > > > 4) Remove __ipt_mutex_up() and __ipt_find_target_lock() which weren't > > used (even in patch-o-matic AFAICT). > > Those are used by ipt action (net/sched/ipt.c). > > Readds __ipt_find_target_lock() and __ipt_mutex_up() and adapts ipt.c > to correctly use it. module must be given back properly upon cleanup. > removes the comments demanding for proper module refcnt since > find_target_lock() takes care of this now. OK, thanks. You missed a module_put in the check_entry-failed error path though. I looked harder at this, and I prefer this patch: Name: Restore net/sched/ipt.c After iptables Kmod Cleanup Status: Tested under nfsim Signed-off-by: Rusty Russell Thomas Graf points out that I broke net/sched/ipt.c when I removed __ipt_find_target_lock. In fact, those routines don't need to keep the lock held, so we can simplify them, and expose an interface (ipt_find_target) which does module loading correctly for net/sched/ipt.c. As Thomas points out, this also gets the module refcounts right. Index: linux-2.6.10-bk8-Netfilter/net/ipv4/netfilter/ip_tables.c =================================================================== --- linux-2.6.10-bk8-Netfilter.orig/net/ipv4/netfilter/ip_tables.c 2005-01-06 09:12:01.000000000 +1100 +++ linux-2.6.10-bk8-Netfilter/net/ipv4/netfilter/ip_tables.c 2005-01-06 09:49:10.000000000 +1100 @@ -430,62 +430,63 @@ return NULL; } -/* Find match, grabs mutex & ref. Returns ERR_PTR() on error. */ -static inline struct ipt_match *find_match_lock(const char *name, u8 revision) +/* Find match, grabs ref. Returns ERR_PTR() on error. */ +static inline struct ipt_match *find_match(const char *name, u8 revision) { struct ipt_match *m; - int found = 0; + int err = 0; if (down_interruptible(&ipt_mutex) != 0) return ERR_PTR(-EINTR); list_for_each_entry(m, &ipt_match, list) { if (strcmp(m->name, name) == 0) { - found = 1; if (m->revision == revision) { - if (!try_module_get(m->me)) - found = 0; - else + if (try_module_get(m->me)) { + up(&ipt_mutex); return m; - } + } + } else + err = -EPROTOTYPE; /* Found something. */ } } up(&ipt_mutex); - - /* Not found at all? NULL so try_then_request_module loads module. */ - if (!found) - return NULL; - - return ERR_PTR(-EPROTOTYPE); + return ERR_PTR(err); } -/* Find target, grabs mutex & ref. Returns ERR_PTR() on error. */ -static inline struct ipt_target *find_target_lock(const char *name, u8 revision) +/* Find target, grabs ref. Returns ERR_PTR() on error. */ +static inline struct ipt_target *find_target(const char *name, u8 revision) { struct ipt_target *t; - int found = 0; + int err = 0; if (down_interruptible(&ipt_mutex) != 0) return ERR_PTR(-EINTR); list_for_each_entry(t, &ipt_target, list) { if (strcmp(t->name, name) == 0) { - found = 1; if (t->revision == revision) { - if (!try_module_get(t->me)) - found = 0; - else + if (try_module_get(t->me)) { + up(&ipt_mutex); return t; - } + } + } else + err = -EPROTOTYPE; /* Found something. */ } } up(&ipt_mutex); + return ERR_PTR(err); +} - /* Not found at all? NULL so try_then_request_module loads module. */ - if (!found) - return NULL; +struct ipt_target *ipt_find_target(const char *name, u8 revision) +{ + struct ipt_target *target; - return ERR_PTR(-EPROTOTYPE); + target = try_then_request_module(find_target(name, revision), + "ipt_%s", name); + if (IS_ERR(target) || !target) + return NULL; + return target; } static int match_revfn(const char *name, u8 revision, int *bestp) @@ -708,15 +709,14 @@ { struct ipt_match *match; - match = try_then_request_module(find_match_lock(m->u.user.name, - m->u.user.revision), + match = try_then_request_module(find_match(m->u.user.name, + m->u.user.revision), "ipt_%s", m->u.user.name); if (IS_ERR(match) || !match) { duprintf("check_match: `%s' not found\n", m->u.user.name); return match ? PTR_ERR(match) : -ENOENT; } m->u.kernel.match = match; - up(&ipt_mutex); if (m->u.kernel.match->checkentry && !m->u.kernel.match->checkentry(name, ip, m->data, @@ -754,8 +754,8 @@ goto cleanup_matches; t = ipt_get_target(e); - target = try_then_request_module(find_target_lock(t->u.user.name, - t->u.user.revision), + target = try_then_request_module(find_target(t->u.user.name, + t->u.user.revision), "ipt_%s", t->u.user.name); if (IS_ERR(target) || !target) { duprintf("check_entry: `%s' not found\n", t->u.user.name); @@ -763,7 +763,6 @@ goto cleanup_matches; } t->u.kernel.target = target; - up(&ipt_mutex); if (t->u.kernel.target == &ipt_standard_target) { if (!standard_check(t, size)) { Index: linux-2.6.10-bk8-Netfilter/net/sched/ipt.c =================================================================== --- linux-2.6.10-bk8-Netfilter.orig/net/sched/ipt.c 2004-12-28 12:31:11.000000000 +1100 +++ linux-2.6.10-bk8-Netfilter/net/sched/ipt.c 2005-01-06 09:37:01.000000000 +1100 @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -60,32 +61,23 @@ struct ipt_target *target; int ret = 0; struct ipt_entry_target *t = p->t; - target = __ipt_find_target_lock(t->u.user.name, &ret); + target = ipt_find_target(t->u.user.name, t->u.user.revision); if (!target) { printk("init_targ: Failed to find %s\n", t->u.user.name); return -1; } DPRINTK("init_targ: found %s\n", target->name); - /* we really need proper ref counting - seems to be only needed for modules?? Talk to laforge */ -/* if (target->me) - __MOD_INC_USE_COUNT(target->me); -*/ t->u.kernel.target = target; - __ipt_mutex_up(); - if (t->u.kernel.target->checkentry && !t->u.kernel.target->checkentry(p->tname, NULL, t->data, t->u.target_size - sizeof (*t), p->hook)) { -/* if (t->u.kernel.target->me) - __MOD_DEC_USE_COUNT(t->u.kernel.target->me); -*/ DPRINTK("ip_tables: check failed for `%s'.\n", t->u.kernel.target->name); + module_put(t->u.kernel.target->me); ret = -EINVAL; } @@ -235,8 +227,12 @@ { struct tcf_ipt *p; p = PRIV(a,ipt); - if (NULL != p) + if (NULL != p) { + struct ipt_entry_target *t = p->t; + if (t && t->u.kernel.target) + module_put(t->u.kernel.target->me); return tcf_hash_release(p, bind); + } return 0; } Index: linux-2.6.10-bk8-Netfilter/include/linux/netfilter_ipv4/ip_tables.h =================================================================== --- linux-2.6.10-bk8-Netfilter.orig/include/linux/netfilter_ipv4/ip_tables.h 2005-01-06 09:11:56.000000000 +1100 +++ linux-2.6.10-bk8-Netfilter/include/linux/netfilter_ipv4/ip_tables.h 2005-01-06 09:40:03.000000000 +1100 @@ -456,6 +456,9 @@ struct module *me; }; +/* net/sched/ipt.c: Gimme access to your targets! Gets target->me. */ +extern struct ipt_target *ipt_find_target(const char *name, u8 revision); + extern int ipt_register_table(struct ipt_table *table); extern void ipt_unregister_table(struct ipt_table *table); extern unsigned int ipt_do_table(struct sk_buff **pskb, -- A bad analogy is like a leaky screwdriver -- Richard Braakman