RFC: ipfw nat VIMAGE improvements

[Marko Zec]

On Wednesday 14 August 2013 22:43:05 Mikolaj Golub wrote:
> On Wed, Aug 14, 2013 at 05:28:31PM +0200, Marko Zec wrote:
> > On Sunday 11 August 2013 22:01:12 Mikolaj Golub wrote:
> > > Hi,
> > >
> > > I would like to commit this patch that fixes some issues related to
> > > ipfw nat module load/unload on VIMAGE featured system.
> > >
> > > Any comments, objections?
> >
> > Far from being an expert in ipfw, I'm worried that the proposed
> > approach of simultaneously acquiring locks on _all_ ipfw instances
> > might be calling for trouble:
> >
> > +       VNET_LIST_RLOCK();
> > +       VNET_FOREACH(vnet_iter) {
> > +               CURVNET_SET(vnet_iter);
> > +               IPFW_WLOCK(&V_layer3_chain);
> > +               CURVNET_RESTORE();
> > +       }
> >         ipfw_nat_ptr = ipfw_nat;
> >         lookup_nat_ptr = lookup_nat;
> >         ipfw_nat_cfg_ptr = ipfw_nat_cfg;
> >         ipfw_nat_del_ptr = ipfw_nat_del;
> >         ipfw_nat_get_cfg_ptr = ipfw_nat_get_cfg;
> >         ipfw_nat_get_log_ptr = ipfw_nat_get_log;
> > -       IPFW_WUNLOCK(&V_layer3_chain);
> > -       V_ifaddr_event_tag = EVENTHANDLER_REGISTER(
> > +       VNET_FOREACH(vnet_iter) {
> > +               CURVNET_SET(vnet_iter);
> > +               IPFW_WUNLOCK(&V_layer3_chain);
> > +               CURVNET_RESTORE();
> > +       }
> > +       VNET_LIST_RUNLOCK();
> >
> > Why couldn't we introduce a per-vnet flag, say V_ipfw_nat_ready, and
> > use it as
> >
> > #define IPFW_NAT_LOADED (V_ipfw_nat_ready)
> >
> > instead of current version of that macro:
> >
> > #define IPFW_NAT_LOADED (ipfw_nat_ptr != NULL)
> >
> > I.e., perhaps in ipfw_nat_init() we could first set all the function
> > pointers, and then iterate over all vnets and set V_ipfw_nat ready
> > there. In ipfw_nat_destroy() we would first iterate over all vnets to
> > clear the flag, before clearing function pointers?
>
> I like you approach. Though insted of iterating vnets in
> ipfw_nat_init/destroy I think it is safe just to set/unset
> V_ipfw_nat_ready in vnet_ipfw_nat_init/uninit.

Yup that should be safe, provided you're 100% sure that 
vnet_ipfw_nat_uninit() fires before ipfw_nat_destroy() - admittedly my 
sysinit ordering insight got a bit rusty so I can't tell at the first 
glance...

Anyhow, this looks fine to me...

Marko