Date: Sun, 9 Jan 2011 23:58:08 +0100 From: joris dedieu <joris.dedieu@gmail.com> To: freebsd-hackers <freebsd-hackers@freebsd.org> Subject: Re: binding non local ip. Message-ID: <AANLkTikBkEw5XU75hX-2gvqiA-JWhqh6pxiMM_UEJdMM@mail.gmail.com> In-Reply-To: <whUqbEc0GlwM0C01ICP8qimcjRQ@hXkfnLhK1nGJouHuoYJSDAyzreM> References: <AANLkTimJBkTdgs4P=XjHyTCinfCOn0Ku8bEVcR-q=Dzc@mail.gmail.com> <I0E3jOweM2JFYO1RRyeZ3hfTJPY@5UW8B3LwPEGInFShts5A7R/S2Yo> <AANLkTinHZNWDwS%2B2fiLfuDUAjOHCHrO4acR8RxZVpudm@mail.gmail.com> <whUqbEc0GlwM0C01ICP8qimcjRQ@hXkfnLhK1nGJouHuoYJSDAyzreM>
next in thread | previous in thread | raw e-mail | index | archive | help
2011/1/9 Eygene Ryabinkin <rea@freebsd.org>:
Sorry for my mail client broken that do not send mails to the list :)
I'll take care.
> Joris, good day.
>
> Sun, Jan 09, 2011 at 06:29:20PM +0100, joris dedieu wrote:
>> Thanks Eygene for this greate review !
>
> No problems ;))
>
>> 2011/1/7 Eygene Ryabinkin <rea@freebsd.org>:
>> > Fri, Jan 07, 2011 at 01:57:21PM +0100, joris dedieu wrote:
>> >> What do you think about it ?
>> > [...]
>> >> +static int =A0 =A0 bindany =3D 0; /* 1 allows to bind a non local ip=
*/
>> >> +SYSCTL_INT(_net_inet_ip, OID_AUTO, bindany, CTLFLAG_RW, &bindany, 0,
>> >> + =A0 =A0"Allow to bind a non local ip");
>> >
>> > On at least 8.x, you will likely want to use VNET_* macros to enable
>> > your new sysctl to be virtualized. =A0Something like this:
>> > {{{
>> > VNET_DEFINE(int, inp_bindany) =3D 0;
>> > SYSCTL_VNET_INT(_net_inet_ip, OID_AUTO, bindany, CTLFLAG_RW,
>> > =A0 =A0 =A0 =A0&VNET_NAME(inp_bindany), 0, "Force INP_BINDANY on all s=
ockets");
>> > }}}
>> > and use VNET(inp_bindany) in subsequent code.
>> Ok it make sense. I will use VNET_*. There are a lot of SYSCTL_* in
>> netinet and netinet6. Is changing this for VNET_* an open task?
>
> I think that the most of them that are applicable to VNET were
> already converted. =A0It is better to ask at freebsd-net@freebsd.org.
>
>> Greate. It makes me understand the way a lot of things are written.
>> Avoid branching if you can.
>> I see that OPSET macro in netinet/ip_output.c lock the inp struct. Is
>> there a need of it there ?
>
> Yes. =A0I had overlooked the need of locking here, sorry.
I wrote a better patch that avoid locking and inp struct modification.
diff --git a/sys/netinet/in_pcb.c b/sys/netinet/in_pcb.c
index d742887..f41e4da 100644
--- a/sys/netinet/in_pcb.c
+++ b/sys/netinet/in_pcb.c
@@ -321,6 +321,9 @@ in_pcbbind(struct inpcb *inp, struct sockaddr
*nam, struct ucred *cred)
*
* On error, the values of *laddrp and *lportp are not changed.
*/
+VNET_DEFINE(int, inp_bindany) =3D 0;
+SYSCTL_VNET_INT(_net_inet_ip, OID_AUTO, bindany, CTLFLAG_RW,
+ &VNET_NAME(inp_bindany), 0, "Force INP_BINDANY on all sockets");
int
in_pcbbind_setup(struct inpcb *inp, struct sockaddr *nam, in_addr_t *laddr=
p,
u_short *lportp, struct ucred *cred)
@@ -392,7 +395,8 @@ in_pcbbind_setup(struct inpcb *inp, struct
sockaddr *nam, in_addr_t *laddrp,
* If INP_BINDANY is set, then the socket may be bo=
und
* to any endpoint address, local or not.
*/
- if ((inp->inp_flags & INP_BINDANY) =3D=3D 0 &&
+ if (VNET(inp_bindany) =3D=3D 0 &&
+ (inp->inp_flags & INP_BINDANY) =3D=3D 0 &&
ifa_ifwithaddr_check((struct sockaddr *)sin) =
=3D=3D 0)
return (EADDRNOTAVAIL);
}
diff --git a/sys/netinet/in_pcb.h b/sys/netinet/in_pcb.h
index 4ba19e6..3720121 100644
--- a/sys/netinet/in_pcb.h
+++ b/sys/netinet/in_pcb.h
@@ -467,6 +467,7 @@ VNET_DECLARE(int, ipport_randomcps);
VNET_DECLARE(int, ipport_randomtime);
VNET_DECLARE(int, ipport_stoprandom);
VNET_DECLARE(int, ipport_tcpallocs);
+VNET_DECLARE(int, inp_bindany);
#define V_ipport_reservedhigh VNET(ipport_reservedhigh)
#define V_ipport_reservedlow VNET(ipport_reservedlow)
diff --git a/sys/netinet/raw_ip.c b/sys/netinet/raw_ip.c
index c91d4a9..17a2e78 100644
--- a/sys/netinet/raw_ip.c
+++ b/sys/netinet/raw_ip.c
@@ -897,6 +897,7 @@ rip_bind(struct socket *so, struct sockaddr *nam,
struct thread *td)
if (TAILQ_EMPTY(&V_ifnet) ||
(addr->sin_family !=3D AF_INET && addr->sin_family !=3D AF_IMPL=
INK) ||
(addr->sin_addr.s_addr &&
+ VNET(inp_bindany) =3D=3D 0 &&
(inp->inp_flags & INP_BINDANY) =3D=3D 0 &&
ifa_ifwithaddr_check((struct sockaddr *)addr) =3D=3D 0))
return (EADDRNOTAVAIL);
>
>> Do you mean there is a way to control user input (ie 0 or 42, but
>> nothing else)?
>
> No, I meant that if you'll use the custom sysctl value handler (via
> SYSCTL_VNET_PROC, not vie SYSCTL_VNET_INT), then you can convert any
> non-zero value to INP_BINDANY and zero to zero. =A0But given the need of
> locking, I don't think that this won't be good to take this road: one
> simple non-conditional logical instruction will be harmless even if it
> is executed when it is not needed; but the block of
> lock-logicalop-unlock will be worse.
> --
> Eygene Ryabinkin =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0,,,^..^,,,
> [ Life's unfair - but root password helps! =A0 =A0 =A0 =A0 =A0 | codelabs=
.ru ]
> [ 82FE 06BC D497 C0DE 49EC =A04FF0 16AF 9EAE 8152 ECFB | freebsd.org ]
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTikBkEw5XU75hX-2gvqiA-JWhqh6pxiMM_UEJdMM>