On 14/06/16(Tue) 13:49, Jeremie Courreges-Anglas wrote: > Vincent Gross <[email protected]> writes: > > On Mon, 13 Jun 2016 19:57:15 +0200 > > Jeremie Courreges-Anglas <[email protected]> wrote: > >> Vincent Gross <[email protected]> writes: > >> > Le Mon, 13 Jun 2016 07:35:16 +0200, > >> > [email protected] (Jeremie Courreges-Anglas) a écrit : > >> >> [email protected] (Jeremie Courreges-Anglas) writes: > >> >> > >> >> > cc'ing sthen since he also has interest in IP_SENDSRCADDR > >> >> > > >> >> > Jeremie Courreges-Anglas <[email protected]> writes: > >> >> > > >> >> >> Vincent Gross <[email protected]> writes: > >> >> >> > >> >> >>> This diff adds support for IP_SENDSRCADDR cmsg on UDP sockets. > >> >> >>> As for udp6_output(), we check that the source address+port is > >> >> >>> available only if inp_laddr != * > >> >> >> > >> >> >> Your last IP_SENDSRCADDR diff didn't have that check, I think > >> >> >> it is harmful. If the socket is not bound then there is > >> >> >> effectively no check performed by in_pcbaddrisavail(), thus I > >> >> >> can use any random address. Other than this additional bypass > >> >> >> check, your diff looks good to me. > >> >> >> > >> > [...] > >> >> >> > >> >> >> I haven't checked yet whether udp6_output is also affected. If > >> >> >> you folks already know that it isn't, please let me know. > >> >> > >> >> The answer is "no", a few tests can't trigger the same problem. > >> >> IIUC in6_selectsrc is responsible for rejection of non-local > >> >> systems. Maybe we should take the same approach in netinet/, and > >> >> extend in_selectsrc()? > >> >> > >> >> -- > >> > > >> > While validating source address inside selection functions is the > >> > right direction, I don't think it would be a good thing to extend > >> > further in_selectsrc() prototype. > >> > >> I find it nice to have all the source address selection in one place. > >> Or do you have another refactoring in mind? > >> > > > > Uh, turns out I was operating on obsolete data. I would actually be > > easy to shrink in_selectsrc() prototype to > > (int)(struct in_addr **, struct sockaddr_in *, struct in_pcb *). > > But this looks like a layering violation to me ... What do you think ? > > I wouldn't mind adding an extra argument and staying closer to > in6_selectsrc.
Can we get the feature in first then see how we refactor things later? I really don't think that extending in_selectsrc() is a good thing. in6_selectsrc() is a bad example and IMHO this function should be revisited. Having a function taking 7 arguments with 4 optional ones makes it very difficult to understand what's happening.
