ipfw cached ucred patch

[Christian S.J. Peron]

Agreed,

This was a concern for me as well, I was pretty carefull about
managing the reference counts, I am currently testing this patch
with a variety of rule types to check for ucred leaks. If/before this patch 
gets committed, I plan on doing another carefull scrutinization
of the ipfw code to make sure there are no return, continues or breaks
etc which could cause the ucred to be leaked.

 
On  2 Jun 2004 Andre Oppermann wrote:
> 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
> 

-- 
Christian S.J. Peron
csjp at FreeBSD.ORG
FreeBSD Committer