Date: Wed, 02 Jun 2004 23:23:02 +0200 From: Andre Oppermann <andre@freebsd.org> To: "Christian S.J. Peron" <csjp@freebsd.org> Cc: ipfw@freebsd.org Subject: Re: ipfw cached ucred patch Message-ID: <40BE4536.4050905@freebsd.org> In-Reply-To: <20040602135155.GA31642@freefall.freebsd.org> References: <20040602043537.GA42327@freefall.freebsd.org> <40BD9D3F.7090100@freebsd.org> <20040602135155.GA31642@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Christian S.J. Peron wrote: > On 2 Jun 2004 Andre Oppermann wrote: > >>Christian S.J. Peron wrote: >> >>>All, >>> >>>Currently, when you have any rules which contain UID/GID >>>constraints, ipfw will lock the pcb hash and do a lookup >>>to find the pcb associated with that packet -- >>>One for each constraint. >>> >>>I have written a patch in attempt to minimize the impact >>>of PCB related lookups for these type of firewall rules. >>> >>>This patch will have the following effects on firewalls which >>>contain UID/GID constraints: >>> >>>o Greatly reduce the locking contention associated >>> with PCB lookups. >>> >>>o Increase the performance of firewall in general by making >>> PCB lookups O(1) rather than O(n) (where n represents >>> number of UID/GID constraints in the ruleset) >>> >>>It would be greatly appriciated if people who are running ipfw >>>rules sets containing UID/GID constraints tested this patch >>>and reported any success or failures. >>> >>>The patch can be downloaded from: >>> >>>http://people.freebsd.org/~csjp/ip_fw2_cached_ucred.patch >> >>You can optimize it even further by directly copying the uid/gid >>from the ucred while you hold the INP_LOCK. There is no need to >>hold on to the entire ucred. It should be sufficient to do the >>ucred lookup only once per packet in the ipfw code. If you don't >>find an INPCB for the packet you'll do a negative lookup for every >>uid/gid rule. > > I thought about this to, however in order to implement GID contraints > properly, we need to use groupmember(9) which requires the > entire cr_groups[16] located in the ucred. I thought it was more > elegant and cheaper to avoid the memcpy(sizeof(gid_t) * NGROUPS) > and stick with the mutex. I see. Hmmm... Actually I'm only concerned that someone later misses a crfree() call and starts to leak ucred structures. ipfw is not the first place you are going to look for it. The more you can keep together in one place the better it is. This kind of error has already happend once with the initial implementation of verrevpath in ifpw. The fuction did not correctly do the ref- counting leading to a hefty rtentry leak. -- Andre
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?40BE4536.4050905>