From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: diff: setting M_BCAST flag
To: YASUOKA Masahiko <yasuoka@openbsd.org>
Cc: tech@openbsd.org
Date: Wed, 12 Mar 2025 20:09:22 +0100

On Wed, Mar 12, 2025 at 09:44:53AM +0900, YASUOKA Masahiko wrote:
> On Tue, 11 Mar 2025 14:48:28 +0100
> Alexander Bluhm <alexander.bluhm@gmx.net> wrote:
> > On Tue, Mar 11, 2025 at 05:42:30PM +0900, YASUOKA Masahiko wrote:
> >> On Mon, 10 Mar 2025 19:28:25 +0100
> >> Alexander Bluhm <bluhm@openbsd.org> wrote:
> >> > On Sat, Mar 08, 2025 at 12:54:35PM +0900, YASUOKA Masahiko wrote:
> >> >> Hello,
> >> >> 
> >> >> Our ip_input() doesn't set M_BCAST flag for 0.0.0.0 or 255.255.255.255
> >> >> if it's not set by the lower layer.  The flag must be set since it is
> >> >> used by the upper layers.  (eg. TCP use the flag to drop the packet.)
> >> >> 
> >> >> The modification on in_ouraddr() is to set M_BCAST flag consistently
> >> >> if the packet is for directed broadcast.  Previous the flag isn't set
> >> >> if the packet is diverted by pf(4).
> >> >> 
> >> >> (eg. on the host that runs a transparent proxy for 80/tcp, when it
> >> >> receives a packet for  directed broadcast but its destination mac
> >> >> address is not ff:ff:ff:ff:ff:ff (eg. wrong netmask), TCP wil not drop
> >> >> it and the connection will  be established.)
> >> >> 
> >> >> ok? comments?
> >> > 
> >> > M_BCAST looks like a layer 2 flag to me.  I we want to prevent layer
> >> > 3 broadcast addresses in TCP, tcp_input() should care about it.
> >> 
> >> Actually, sys/sys/mbuf.h says the flags is for link-level
> >> 
> >> #define M_BCAST         0x0100  /* sent/received as link-level broadcast */
> >> 
> >> But, see the commit below,
> > 
> > You are right.  Removing the check in UDP, but not adding it in IP
> > was wrong.  At least the broadcast flag in ip_input() was forgotten.
> > 
> > The comment /* Make sure M_BCAST is set */ does not make sense.  It
> > just describes the simple statement witten in the C code.  Either
> > remove the comment, or explain why M_BCAST has to be set.
> > 
> > Do not move pf_ouraddr() after route_mpath().  That counteracts the
> > idea of this function.  If pf is smart and knows the packet is for
> > us, we don't need a route lookup.
> 
> I see. The judgment by pf is prior to the other IP layer logic. Then
> I'd like to move checking for 255.255.255.255 after the pf_ouraddr()
> so that setting the flag is always done after the judgment by pf.

Yes, that order is better.

> ok?

OK bluhm@

In a previous diff you added a missing space between switch and (
	switch(in_ouraddr(m, ifp, ro, flags))
This is also correct.

> Index: sys/netinet/ip_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> diff -u -p -r1.404 ip_input.c
> --- sys/netinet/ip_input.c	2 Mar 2025 21:28:32 -0000	1.404
> +++ sys/netinet/ip_input.c	12 Mar 2025 00:21:42 -0000
> @@ -506,12 +506,6 @@ ip_input_if(struct mbuf **mp, int *offp,
>  		goto bad;
>  	}
>  
> -	if (ip->ip_dst.s_addr == INADDR_BROADCAST ||
> -	    ip->ip_dst.s_addr == INADDR_ANY) {
> -		nxt = ip_ours(mp, offp, nxt, af, ns);
> -		goto out;
> -	}
> -
>  	if (ns == NULL) {
>  		ro = &iproute;
>  		ro->ro_rt = NULL;
> @@ -864,6 +858,12 @@ in_ouraddr(struct mbuf *m, struct ifnet 
>  #endif
>  
>  	ip = mtod(m, struct ip *);
> +
> +	if (ip->ip_dst.s_addr == INADDR_BROADCAST ||
> +	    ip->ip_dst.s_addr == INADDR_ANY) {
> +		m->m_flags |= M_BCAST;
> +		return (1);
> +	}
>  
>  	rt = route_mpath(ro, &ip->ip_dst, &ip->ip_src, m->m_pkthdr.ph_rtableid);
>  	if (rt != NULL) {