kern/179901: [netinet] [patch] Multicast SO_REUSEADDR handled incorrectly

[Michael Gmelin]

The following reply was made to PR kern/179901; it has been noted by GNATS.

From: Michael Gmelin <freebsd at grem.de>
To: Mikolaj Golub <trociny at FreeBSD.org>
Cc: bug-followup at FreeBSD.org
Subject: Re: kern/179901: [netinet] [patch] Multicast SO_REUSEADDR handled
 incorrectly
Date: Tue, 25 Jun 2013 13:39:38 +0200

 On Mon, 24 Jun 2013 23:29:42 +0300
 Mikolaj Golub <trociny at FreeBSD.org> wrote:
 
 > Michael,
 
 Hi,
 > 
 > Thank you for your analysis and the patch.
 > 
 > I have the following notes to your patch though:
 > 
 > 1) INET6 needs fixing too.
 Yes, but it seems like your patch is fixing the not all places in
 in6_pcb.c, I think you should modify the code at line 246 as well:
 
                         } else if (t && (reuseport == 0 ||
                             (t->inp_flags2 & INP_REUSEPORT) == 0)) {
                                 return (EADDRINUSE);
                         }
 
 so it says
                         } else if (t &&
                             (reuseport & inp_so_options(t)) == 0) {
 		
 Instead you're modifying the code at line 265 (which also seems to be
 affected, so it makes sense).
 
 > 
 > 2) It looks like after introducing INP_REUSEADDR there is no need in
 > handling the IN_MULTICAST case in ip_ctloutput().
 I think so too, in the end REUSEADDR and REUSEPORT are set
 independently now and REUSEADDR only gets its special meaning when being
 checked during binding. Therefore the code in ip_output should always
 do the right thing.
 
 > 
 > 3) Actually you don't have to use IN_MULTICAST() in
 > in_pcbbind_setup(): the information is already encoded in reuseport
 > variable.
 That seems pretty implicit to me, but it also keeps the original logic
 more or less intact. Whatever works best for you I guess.
 
 > 
 > 4) The patch not only fixes the regression introduced by r227207, but
 > also changes the historical behavior before r227207. Although the
 > change might be correct it is better to separate these issues. Feeling
 > guilty for the regression introduced in r227207 I am eager to fix it
 > ASAP, before 9.2 release. But I don't have strong opinion about
 > changing the historical behavior.
 
 I have a hard time believing anybody relied on the previous behaviour,
 so I wouldn't try to resemble it.
 
 > 
 > So, could you please look at the attached patch, which is based on
 > your idea of INP_REUSEADDR flag? Now the code more resembles the code
 > before r227207 in looks and I am a little more confident that there is
 > no regression.
 > 
 > I would appreciate any testing. Note, it is against CURRENT; STABLE
 > will require patching in_pcb.h manually due to newly introduced
 > INP_FREED flag.
 > 
 
 Once 1) has been resolved I can test on a machine running 9.1-RELEASE
 later (the patch is small enough to apply it manually). I will run the
 "unit test" code from multicast.c I sent earlier and add IPv6 test
 cases to it as well.
 
 Cheers,
 Michael
 
 
 -- 
 Michael Gmelin