On Thu, Dec 16, 2021 at 11:48:58AM -0700, Theo de Raadt wrote:
> 'route sourceaddr' support is incomplete.
> In particular it does not work in ping or traceroute.

Thanks for the explanation.

> > On Thu, Dec 16, 2021 at 07:20:04PM +0100, Denis Fondras wrote:
> > > Raw sockets do not comply with route sourceaddr.

So it is about unconnected sockets.  Should it be fixed there?

> > > + struct sockaddr *ip4_source = NULL;

Initialization = NULL is not necessay.

> > > +                 ip4_source = rtable_getsource(ro->ro_tableid, AF_INET);
> > > +                 if (ip4_source != NULL) {
> > > +                         struct ifaddr *ifa;
> > > +                         if ((ifa = ifa_ifwithaddr(ip4_source,

I would not call a function from the IP output path that grabs
kernel lock and loops over all interfaces and addresses.

Should we store it at the socket and use it if inp_laddr is not
set?

struct art_root {
        struct sockaddr         *source;        /* [K] optional src addr to use 
*/
Why does this field not have an ar_ prefix like all the others?
Why does it need kernel lock?  You don't have kernel lock here.
It is set with kernel lock, cleared with net lock and unset with
the routing socket lock.  The memory of the sockaddr seems to
be protected by ifaddr refcounting and net lock.

Can we assume that a store to a pointer and also dereferencing it
is atomic?

> > > +                             ro->ro_tableid)) != NULL &&
> > > +                             ISSET(ifa->ifa_ifp->if_flags, IFF_UP)) {
> > > +                                 ip->ip_src = 
> > > satosin(ip4_source)->sin_addr;
> > > +                         }
> > > +                 } else
> > > +                         ip->ip_src = ia->ia_addr.sin_addr;
> > > +         }
> > >   }
> > >  
> > >  #ifdef IPSEC
> > 

Reply via email to