Question about in_pcbbind() and in6_pcbbind()

[Elad Efrat <elad%NetBSD.org@localhost>]

Hi,

There are three issues that I'm wondering about regarding the two
functions mentioned in the subject:

1. When called from netinet/tcp_usrreq.c:tcp_usrrreq(), in_pcbbind()
   takes "l", but in6_pcbbind() takes NULL:

    362         case PRU_LISTEN:
    363 #ifdef INET
    364                 if (inp && inp->inp_lport == 0) {
    365                         error = in_pcbbind(inp, NULL, l);
    366                         if (error)
    367                                 break;
    368                 }
    369 #endif
    370 #ifdef INET6
    371                 if (in6p && in6p->in6p_lport == 0) {
    372                         error = in6_pcbbind(in6p, NULL, NULL);
    373                         if (error)
    374                                 break;
    375                 }
    376 #endif
    377                 tp->t_state = TCPS_LISTEN;
    378                 break;

   Is there a reason for this? it seems that the use for both is the
   same -- check if a privileged port can be bound to, only that for the
   inet6 case, the request is denied if l == NULL.

   I'd like us to always pass an lwp argument, and in the future
   probably just the kauth_cred_t.

2. When called from the same locations, you'll also notice the second
   argument (mbuf *nam) is NULL. Passing NULL is functionally equivalent
   to passing a zeroed struct sockaddr_in6 wrapped in an mbuf, with one
   difference: there's no sockaddr struct to pass to kauth(9). (We could
   just pass the address and the port, but I think we'll be wasting
   parameter space that way.)

   I believe this is something we want to change, as we do want policies
   that will cover all common (bind()) as well as uncommon (LOWPORT +
   connect) cases that can lead to an address/port bind.

   My two proposed solutions:

     - Make the caller construct a dummy mbuf with a dummy sockaddr_in
       and sockaddr_in6 and pass them when calling in_pcbbind() and
       in6_pcbbind respectively.

     or

     - Put code inside in_pcbbind() and in6_pcbbind() to cope with a
       nam == NULL by creating a dummy sockaddr_in and sockaddr_in6
       themselves before proceeding.

   Which one is preferable?

   (Personally, I think we should also slice the logic inside both
   functions to two, smaller functions, say in{,6}_pcbbind_{addr,port},
   if only to make reading of the code easier. The original functions
   will retain original functionality by calling both new ones, of
   course.)

3. It seems that in_pcbbind(), when exhausting all free ports when
   auto-assigning a port, does the following:

    356  if (!in_nullhost(inp->inp_laddr))
    357          inp->inp_laddr.s_addr = INADDR_ANY;
    358  return (EAGAIN);

   To prevent a case where an inpcb will have an address but not a port
   (that makes sense; see the test for port/non-"null" address on entry
   to both in_pcbbind() and in6_pcbbind()), while in6_pcbbind() does
   not:

    871                 if (t == 0)
    872                         goto found;
    873         }
    874
    875         return (EAGAIN);
    876
    877 found:
    878         in6p->in6p_flags |= IN6P_ANONPORT;

   Is this intended? if yes, why?

Thanks,

-e.