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.
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?
> 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?
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);
