On Sat, Mar 19, 2022 at 10:57:18PM +1000, David Gwynne wrote:
> On Sat, Mar 19, 2022 at 12:53:54PM +0100, Claudio Jeker wrote:
> > On Sat, Mar 19, 2022 at 12:27:43PM +0100, Claudio Jeker wrote:
> > > On Sat, Mar 19, 2022 at 10:51:13AM +0000, Stuart Henderson wrote:
> > > > On 2022/03/19 19:14, David Gwynne wrote:
> > > > > On Thu, Mar 17, 2022 at 12:05:16PM -0600, Theo de Raadt wrote:
> > > > > > This should not be done in applications. The kernel must do it.
> > > > > > It means
> > > > > > the current kernel code is worng.
> > > > >
> > > > > i think this is the right place for raw ipv4 sockets like what
> > > > > ping/traceroute uses.
> > > > >
> > > > > ipv6 looks like it already does the right thing.
> > > >
> > > > Given that this turned out so similar to the existing v6 support that
> > > > I think you didn't notice before writing the v4 version, this looks
> > > > like the right place indeed :)
> > > >
> > > > Works for me, OK
> > >
> > > Did someone try this on an ospf router?
> > > I think our ospfd(8) always includes the source address and uses
> > > IP_HDRINCL
> > > but not sure about other daemons. Are there other raw IP users that expect
> > > the IP source to be set to the outgoing interface by default?
> > > Maybe IGMP proxies and routers. E.g. dvmrpd depends on IP_MULTICAST_IF to
> > > set the source IP address.
> > >
> > > Looking at the code I think this will break the IP_MULTICAST_IF case.
> > > In ip_output() the check is like this:
> > >
> > > if ((IN_MULTICAST(ip->ip_dst.s_addr) ||
> > > (ip->ip_dst.s_addr == INADDR_BROADCAST)) &&
> > > imo != NULL && (ifp = if_get(imo->imo_ifidx)) != NULL) {
> > >
> > > mtu = ifp->if_mtu;
> > > if (ip->ip_src.s_addr == INADDR_ANY) {
> > > struct in_ifaddr *ia;
> > >
> > > IFP_TO_IA(ifp, ia);
> > > if (ia != NULL)
> > > ip->ip_src = ia->ia_addr.sin_addr;
> > > }
> > >
> > > Now raw_ip.c already set ip_src to something so
> > > if (ip->ip_src.s_addr == INADDR_ANY)
> > > is never true.
> > >
> > > It is possible to skip the in_pcbselsrc() call in rip_output() when
> > > IN_MULTICAST(ip->ip_dst.s_addr) || (ip->ip_dst.s_addr == INADDR_BROADCAST)
> > >
> > > Is that good enough?
> >
> > Actually in_pcbselsrc() checks the multicast options but only for the
> > IN_MULTICAST() case. I guess we could add INADDR_BROADCAST handling in
> > there as well. Seems like a sensible thing todo. Broadcast is just a very
> > special version of multicast.
>
> i know we talked about this off list, but for the record there are two
> kinds of IP broadcast. there's 255.255.255.255, and the broadcast
> address on subnets, eg, 192.168.1.0/24 has a broadcast address of
> 192.168.1.255. you're talking about 255.255.255.255 here.
Yes, this only matters for 255.255.255.255. The per netowrk broadcast
addresses were a stupid idea and luckily not relevant here.
> im honestly surprised in_pcbselsrc doesnt already do what you're talking
> about. like i said, the impression i get from stevens and other bits of
> the stack is that 255.255.255.255 is largely treated as a multicast
> address, which is why im surprised that it isnt handled already and why
> i think it makes sense. what tests do we need to feel confident with it
> though?
I was also surprised that this code differs from the very similar code in
ip_output(). I agree that 255.255.255.255 should be treated like a
multicast address.
I'm fairly confident that this will be fine. It may unbreak some cases but
luckily the use of 255.255.255.255 is very limited. Apart from DHCP/BOOTP
not much depends on it.
> > Also if this is changed is there still a way to have ip->ip_src ==
> > INADDR_ANY in ip_output()?
>
> dunno. would slapping a counter on it be useful?
Something like that. It would be nice to remove some of the code in
ip_output() which should be redundant now. But as usual I guess some other
protocol handler may still depend on this feature of ip_output.
A counter or even printf would help identify these cases.
> Index: in_pcb.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/in_pcb.c,v
> retrieving revision 1.261
> diff -u -p -r1.261 in_pcb.c
> --- in_pcb.c 14 Mar 2022 22:38:43 -0000 1.261
> +++ in_pcb.c 19 Mar 2022 12:54:55 -0000
> @@ -893,11 +893,13 @@ in_pcbselsrc(struct in_addr **insrc, str
> }
>
> /*
> - * If the destination address is multicast and an outgoing
> - * interface has been set as a multicast option, use the
> - * address of that interface as our source address.
> + * If the destination address is multicast or limited
> + * broadcast (255.255.255.255) and an outgoing interface has
> + * been set as a multicast option, use the address of that
> + * interface as our source address.
> */
> - if (IN_MULTICAST(sin->sin_addr.s_addr) && mopts != NULL) {
> + if ((IN_MULTICAST(sin->sin_addr.s_addr) ||
> + sin->sin_addr.s_addr == INADDR_BROADCAST) && mopts != NULL) {
> struct ifnet *ifp;
>
> ifp = if_get(mopts->imo_ifidx);
>
OK claudio@
--
:wq Claudio
