On Sat, Sep 19, 2015 at 07:57:00PM +0200, Vincent Gross wrote:
> On 09/18/15 23:39, David Hill wrote:
> > On Fri, Sep 18, 2015 at 11:05:55PM +0200, Vincent Gross wrote:
> >> On 09/18/15 15:18, David Hill wrote:
> >>> Is this 'if (count)' statement needed? We know first > last, so count
> >>> will always be positive. lastport will always be set.
> >>
> >>> if last == first, then the if statement will be false and lastport will
> >>> be uninitialized, I believe.
> >>>
> >>
> >> Both remarks are true, but I think it is better to keep a more extensive
> >> refactoring in a separate diff, refactoring that shall get rid of this
> >> yucky code duplication.
> >>
> >
> > Well, this code changes the current behavior. I'd at least change
> > lastport to be initialized to 0 to keep the behavior the same. It was
> > previously set to 0 with M_ZERO.
> >
>
> Fixed. Ok ?
LGTM.
>
> Index: sys/netinet/in_pcb.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/in_pcb.c,v
> retrieving revision 1.179
> diff -u -p -r1.179 in_pcb.c
> --- sys/netinet/in_pcb.c 11 Sep 2015 15:29:47 -0000 1.179
> +++ sys/netinet/in_pcb.c 19 Sep 2015 17:52:42 -0000
> @@ -199,7 +199,6 @@ in_pcbinit(struct inpcbtable *table, int
> &table->inpt_lhash);
> if (table->inpt_lhashtbl == NULL)
> panic("in_pcbinit: hashinit failed for lport");
> - table->inpt_lastport = 0;
> table->inpt_count = 0;
> arc4random_buf(&table->inpt_key, sizeof(table->inpt_key));
> }
> @@ -281,8 +280,8 @@ in_pcbbind(struct inpcb *inp, struct mbu
> {
> struct socket *so = inp->inp_socket;
> struct inpcbtable *table = inp->inp_table;
> - u_int16_t *lastport = &inp->inp_table->inpt_lastport;
> struct sockaddr_in *sin;
> + u_int16_t lastport = 0;
> u_int16_t lport = 0;
> int wild = 0, reuseport = (so->so_options & SO_REUSEPORT);
> int error;
> @@ -391,16 +390,16 @@ in_pcbbind(struct inpcb *inp, struct mbu
> */
> count = first - last;
> if (count)
> - *lastport = first - arc4random_uniform(count);
> + lastport = first - arc4random_uniform(count);
>
> do {
> if (count-- < 0) /* completely used? */
> return (EADDRNOTAVAIL);
> - --*lastport;
> - if (*lastport > first || *lastport < last)
> - *lastport = first;
> - lport = htons(*lastport);
> - } while (in_baddynamic(*lastport,
> so->so_proto->pr_protocol) ||
> + --lastport;
> + if (lastport > first || lastport < last)
> + lastport = first;
> + lport = htons(lastport);
> + } while (in_baddynamic(lastport,
> so->so_proto->pr_protocol) ||
> in_pcblookup(table, &zeroin_addr, 0,
> &inp->inp_laddr, lport, wild, inp->inp_rtableid));
> } else {
> @@ -409,16 +408,16 @@ in_pcbbind(struct inpcb *inp, struct mbu
> */
> count = last - first;
> if (count)
> - *lastport = first + arc4random_uniform(count);
> + lastport = first + arc4random_uniform(count);
>
> do {
> if (count-- < 0) /* completely used? */
> return (EADDRNOTAVAIL);
> - ++*lastport;
> - if (*lastport < first || *lastport > last)
> - *lastport = first;
> - lport = htons(*lastport);
> - } while (in_baddynamic(*lastport,
> so->so_proto->pr_protocol) ||
> + ++lastport;
> + if (lastport < first || lastport > last)
> + lastport = first;
> + lport = htons(lastport);
> + } while (in_baddynamic(lastport,
> so->so_proto->pr_protocol) ||
> in_pcblookup(table, &zeroin_addr, 0,
> &inp->inp_laddr, lport, wild, inp->inp_rtableid));
> }
> Index: sys/netinet/in_pcb.h
> ===================================================================
> RCS file: /cvs/src/sys/netinet/in_pcb.h,v
> retrieving revision 1.89
> diff -u -p -r1.89 in_pcb.h
> --- sys/netinet/in_pcb.h 16 Apr 2015 19:24:13 -0000 1.89
> +++ sys/netinet/in_pcb.h 19 Sep 2015 17:52:42 -0000
> @@ -152,7 +152,6 @@ struct inpcbtable {
> struct inpcbhead *inpt_hashtbl, *inpt_lhashtbl;
> SIPHASH_KEY inpt_key;
> u_long inpt_hash, inpt_lhash;
> - u_int16_t inpt_lastport;
> int inpt_count;
> };
>
> Index: sys/netinet6/in6_pcb.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/in6_pcb.c,v
> retrieving revision 1.74
> diff -u -p -r1.74 in6_pcb.c
> --- sys/netinet6/in6_pcb.c 11 Sep 2015 15:29:47 -0000 1.74
> +++ sys/netinet6/in6_pcb.c 19 Sep 2015 17:52:43 -0000
> @@ -294,7 +294,7 @@ in6_pcbsetport(struct in6_addr *laddr, s
> struct socket *so = inp->inp_socket;
> struct inpcbtable *table = inp->inp_table;
> u_int16_t first, last;
> - u_int16_t *lastport = &inp->inp_table->inpt_lastport;
> + u_int16_t lastport = 0;
> u_int16_t lport = 0;
> int count;
> int wild = INPLOOKUP_IPV6;
> @@ -334,16 +334,16 @@ in6_pcbsetport(struct in6_addr *laddr, s
> */
> count = first - last;
> if (count)
> - *lastport = first - arc4random_uniform(count);
> + lastport = first - arc4random_uniform(count);
>
> do {
> if (count-- < 0) /* completely used? */
> return (EADDRNOTAVAIL);
> - --*lastport;
> - if (*lastport > first || *lastport < last)
> - *lastport = first;
> - lport = htons(*lastport);
> - } while (in_baddynamic(*lastport, so->so_proto->pr_protocol) ||
> + --lastport;
> + if (lastport > first || lastport < last)
> + lastport = first;
> + lport = htons(lastport);
> + } while (in_baddynamic(lastport, so->so_proto->pr_protocol) ||
> in_pcblookup(table, &zeroin6_addr, 0,
> &inp->inp_laddr6, lport, wild, inp->inp_rtableid));
> } else {
> @@ -352,16 +352,16 @@ in6_pcbsetport(struct in6_addr *laddr, s
> */
> count = last - first;
> if (count)
> - *lastport = first + arc4random_uniform(count);
> + lastport = first + arc4random_uniform(count);
>
> do {
> if (count-- < 0) /* completely used? */
> return (EADDRNOTAVAIL);
> - ++*lastport;
> - if (*lastport < first || *lastport > last)
> - *lastport = first;
> - lport = htons(*lastport);
> - } while (in_baddynamic(*lastport, so->so_proto->pr_protocol) ||
> + ++lastport;
> + if (lastport < first || lastport > last)
> + lastport = first;
> + lport = htons(lastport);
> + } while (in_baddynamic(lastport, so->so_proto->pr_protocol) ||
> in_pcblookup(table, &zeroin6_addr, 0,
> &inp->inp_laddr6, lport, wild, inp->inp_rtableid));
> }
>
>
