Re: kern/41114: soft interrupt queue patch for BRIDGE_IPF to prevent lock issues

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Thu, Apr 02, 2009 at 04:08:05PM -0400, Peter wrote:
> On Thu, Apr 2, 2009 at 6:10 AM, Manuel Bouyer 
> <bouyer%antioche.eu.org@localhost> wrote:
> > The following reply was made to PR kern/41114; it has been noted by GNATS.
> >  I don't think it's necessery to have a soft interrupt queue for
> >  bridge_enqueue(). It should already be called only from soft interrupt
> >  of thread context.
> >  bridge_input()/bridge_forward() can use the ifp->if_snd queue, there's
> >  no need to store more bridge information here (it's already a per-interface
> >  queue, which is currently unused for bridge).
> >  See the patch I sent to tech-net which does this.
> >
> I should have checked where enqueue was called from, that makes sense.
>  But isn't it a problem to put packets on the if_snd queue that are
> unfiltered?

if_snd is used as input queue here, not output :)

> 
> >  > If bridge filtering is off, the soft interrupts are not used so there's 
> > no performance penalty.
> >
> >  That may not be a good idea. The problem is not limited to BRIDGE_IPF;
> >  as pointed out by tls@ calling the underlying interface's if_start function
> >  from hard interrupt context may also be troublesome.
> >
> I didn't know this.  I just assumed that if the bridge worked without
> BRIDGE_IPF, then it would work if it was fixed and it would be better
> not to introduce slow downs to anything else.
> 
> >  >
> >  >    stopping and starting the bridge clears if_bridge pointers in 
> > interfaces that have been added to the bridge.  Previously the bridge code 
> > was called even if the bridge was down and then the bridge would either 
> > return or route it.  This seemed strange.  If the bridge is down, the 
> > packet should be handled as normal by that interface.
> >
> >  This is a change in behavior that may have unwanted side effects.
> >  This should be discussed in a separate PR I think.
> >  Note that the current bridge code already checks IFF_RUNNING, so it 
> > effectively
> >  behaves as a non-bridged interface when the bridge is down, even if the
> >  bridge code is called.
> 
> As I said, it seemed strange.  bridge_input checks IFF_RUNNING,
> bridge_output handles the packet.  Why should the bridge code be
> called if the bridge is down?

bridge_output() also checks IFF_RUNNING

> 
> >  >
> >  >    bridge_output previously passed runfilt=0 to bridge_enqueue meaning 
> > that outgoing local packets were not filtered.  This seemed strange so I 
> > put it to 1.  If bridge filtering is on, then you would want it on for 
> > local packets as well I would assume.
> >
> >  ether_output() has already been called in ether_output() for this packet.
> >  I'm not sure this change is a good idea because the same packet would
> >  then be seen 2 times on 2 different interfaces. Again this is a
> >  behavior change that should be discussed separately.
> >
> I don't understand, "ether_output() has already been called in
> either_output()"?  Well, locally originated packets should be
> filtered.

I meant "pfil_hook() has already been called in ether_output()"

> 
> There might however be a major issue with my patch in that "dup-to" in
> ipf seems broken for me in different ways.  I am curious as to what
> might have caused that.  Do you have any guesses?

Maybe you created a recursion by calling pfil_hook() from bridge_output ?

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--