Flow of broadcast/multicast packets in pf when a bridge is present

[Kristof Provost]

> On 28 Dec 2019, at 12:52, Andreas Longwitz <longwitz at incore.de> wrote:
> 
> In the meantime I have understand I was wrong about the code snippet
> 
>>   mc2 = m_dup(m, M_NOWAIT);
>>   if (mc2 != NULL) {
>>      /* Keep the layer3 header aligned */
>>      int i = min(mc2->m_pkthdr.len, max_protohdr);
>>      mc2 = m_copyup(mc2, i, ETHER_ALIGN);
>>   }
>>   if (mc2 != NULL) {
>>      mc2->m_pkthdr.rcvif = bifp;
>>      (*bifp->if_input)(bifp, mc2);
>>   }
> 
> My mistake concerned the function call m_copyup(): The mbuf chain is
> copied correct and not shortened, I was confused because of the field
> m_len in mc2. So reinjecting the packet in the bridge is ok.
> 
> Another aspect is what is done next with the broadcast/multicast packet
> handled by this code:
> 
>> /* Return the original packet for local processing. */
>> return (m);
> 
> Therefore local processing on the member interface is done for
> broadcast/multicast packets without checking the pfil_local_phys
> variable. That was confusing me because these packets are counting twice
> in the pf rules. I think this is needless and pfil_local_phys should
> respect all packets not only unicast.
> 
> After introducing the patch
> 
> --- if_bridge.c.iorig    2019-05-14 09:43:33.000000000 +0200
> +++ if_bridge.c 2019-12-28 11:54:52.000000000 +0100
> @@ -2386,6 +2386,10 @@
>                  (*bifp->if_input)(bifp, mc2);
>          }
> 
> +         if (!pfil_local_phys ) {
> +                 m_freem(m);
> +                 return (NULL);
> +         }
>          /* Return the original packet for local processing. */
>          return (m);
>  }
> 
> everything works fine and all the counters in pf have values as expected
> 
Can you put that in Phabricator and cc me and kevans@? (I seem to remember him touching related code a few months ago).

Thanks,
Kristof