[PATCH] net: Fix a documentation bug wrt. ip_unprivileged_port_start

[PATCH] net: Fix a documentation bug wrt. ip_unprivileged_port_start

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

It cannot overlap with the local port range - ie. with autobind selectable
ports - and not with reserved ports.

Indeed 'ip_local_reserved_ports' isn't even a range, it's a (by default
empty) set.

Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 Documentation/networking/ip-sysctl.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 099a55bd1432..dcf05fd1143a 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -904,8 +904,9 @@ ip_local_port_range - 2 INTEGERS
 	Defines the local port range that is used by TCP and UDP to
 	choose the local port. The first number is the first, the
 	second the last local port number.
-	If possible, it is better these numbers have different parity.
-	(one even and one odd values)
+	If possible, it is better these numbers have different parity
+	(one even and one odd value).
+	Must be greater than or equal to ip_unprivileged_port_start.
 	The default values are 32768 and 60999 respectively.
 
 ip_local_reserved_ports - list of comma separated ranges
@@ -944,7 +945,7 @@ ip_unprivileged_port_start - INTEGER
 	unprivileged port in the network namespace.  Privileged ports
 	require root or CAP_NET_BIND_SERVICE in order to bind to them.
 	To disable all privileged ports, set this to 0.  It may not
-	overlap with the ip_local_reserved_ports range.
+	overlap with the ip_local_port_range.
 
 	Default: 1024
 
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH] net: Fix a documentation bug wrt. ip_unprivileged_port_start

[Jakub Kicinski]

On Fri, 22 Nov 2019 14:12:04 -0800, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
> 
> It cannot overlap with the local port range - ie. with autobind selectable
> ports - and not with reserved ports.
> 
> Indeed 'ip_local_reserved_ports' isn't even a range, it's a (by default
> empty) set.
> 
> Cc: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>

Since this is a documentation _bug_ :) we probably need a Fixes tag.
The mistake is almost 3 years old, could be worth giving the backport
bots^W folks a chance to pick it up.

Is this all the way from 4548b683b781 ("Introduce a sysctl that
modifies the value of PROT_SOCK.") ?

Re: [PATCH] net: Fix a documentation bug wrt. ip_unprivileged_port_start

[Maciej Żenczykowski]

> Since this is a documentation _bug_ :) we probably need a Fixes tag.
> The mistake is almost 3 years old, could be worth giving the backport
> bots^W folks a chance to pick it up.
>
> Is this all the way from 4548b683b781 ("Introduce a sysctl that
> modifies the value of PROT_SOCK.") ?

Yes, indeed.
That commit adds the documentation itself, and:

// ipv4_local_port_range()
-               if (range[1] < range[0])
+               /* Ensure that the upper limit is not smaller than the lower,
+                * and that the lower does not encroach upon the privileged
+                * port limit.
+                */
+               if ((range[1] < range[0]) ||
+                   (range[0] < net->ipv4.sysctl_ip_prot_sock))

and

// ipv4_privileged_ports()

+       pports = net->ipv4.sysctl_ip_prot_sock;
+
+       ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
+
+       if (write && ret == 0) {
+               inet_get_local_port_range(net, &range[0], &range[1]);
+               /* Ensure that the local port range doesn't overlap with the
+                * privileged port range.
+                */
+               if (range[0] < pports)
+                       ret = -EINVAL;
+               else
+                       net->ipv4.sysctl_ip_prot_sock = pports;
+       }

Anyway, I guess this means this commit should have:

Fixes: 4548b683b781 ("Introduce a sysctl that modifies the value of PROT_SOCK.")

(which is in v4.10-rc4-733-g4548b683b781)

Should I resubmit with the new tag, or will you just pick it up?

Re: [PATCH] net: Fix a documentation bug wrt. ip_unprivileged_port_start

[Jakub Kicinski]

On Sun, 24 Nov 2019 01:16:35 -0800, Maciej Żenczykowski wrote:
> > Since this is a documentation _bug_ :) we probably need a Fixes tag.
> > The mistake is almost 3 years old, could be worth giving the backport
> > bots^W folks a chance to pick it up.
> >
> > Is this all the way from 4548b683b781 ("Introduce a sysctl that
> > modifies the value of PROT_SOCK.") ?  
> 
> Yes, indeed.
> That commit adds the documentation itself, and:
> 
> // ipv4_local_port_range()
> -               if (range[1] < range[0])
> +               /* Ensure that the upper limit is not smaller than the lower,
> +                * and that the lower does not encroach upon the privileged
> +                * port limit.
> +                */
> +               if ((range[1] < range[0]) ||
> +                   (range[0] < net->ipv4.sysctl_ip_prot_sock))
> 
> and
> 
> // ipv4_privileged_ports()
> 
> +       pports = net->ipv4.sysctl_ip_prot_sock;
> +
> +       ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> +
> +       if (write && ret == 0) {
> +               inet_get_local_port_range(net, &range[0], &range[1]);
> +               /* Ensure that the local port range doesn't overlap with the
> +                * privileged port range.
> +                */
> +               if (range[0] < pports)
> +                       ret = -EINVAL;
> +               else
> +                       net->ipv4.sysctl_ip_prot_sock = pports;
> +       }
> 
> Anyway, I guess this means this commit should have:
> 
> Fixes: 4548b683b781 ("Introduce a sysctl that modifies the value of PROT_SOCK.")
> 
> (which is in v4.10-rc4-733-g4548b683b781)
> 
> Should I resubmit with the new tag, or will you just pick it up?

Thanks for the tag, now that you showed me the code I'm not 100% sure
the doc is correct :S

The first unprivileged port has to be lower than the
ip_local_port_range, IOW ip_local_port_range must fall 
into unprivileged range entirely.

So does "It may not overlap with the ip_local_port_range." really
express that? Should it say something like "ip_local_port_range must
fall into the unprivileged range" or "It must be lower or equal to start
of ip_local_port_range" or "Unprivileged port range must contain
ip_local_port_range" or...

Re: [PATCH] net: Fix a documentation bug wrt. ip_unprivileged_port_start

[Maciej Żenczykowski]

I'm not entirely certain what would be better.
It does seem sensible to me... but perhaps, it would be better if
'may' was 'must' and 'it' was 'they',
since it's referring to the 'privileged ports' which is plural.

Re: [PATCH] net: Fix a documentation bug wrt. ip_unprivileged_port_start

[Jakub Kicinski]

On Mon, 25 Nov 2019 14:35:11 -0800, Maciej Żenczykowski wrote:
> I'm not entirely certain what would be better.
> It does seem sensible to me... but perhaps, it would be better if
> 'may' was 'must' and 'it' was 'they',
> since it's referring to the 'privileged ports' which is plural.

Ack, sounds good

[PATCH v2] net: Fix a documentation bug wrt. ip_unprivileged_port_start

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

It cannot overlap with the local port range - ie. with autobind selectable
ports - and not with reserved ports.

Indeed 'ip_local_reserved_ports' isn't even a range, it's a (by default
empty) set.

Fixes: 4548b683b781 ("Introduce a sysctl that modifies the value of PROT_SOCK.")
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 Documentation/networking/ip-sysctl.txt | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 099a55bd1432..fd26788e8c96 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -904,8 +904,9 @@ ip_local_port_range - 2 INTEGERS
 	Defines the local port range that is used by TCP and UDP to
 	choose the local port. The first number is the first, the
 	second the last local port number.
-	If possible, it is better these numbers have different parity.
-	(one even and one odd values)
+	If possible, it is better these numbers have different parity
+	(one even and one odd value).
+	Must be greater than or equal to ip_unprivileged_port_start.
 	The default values are 32768 and 60999 respectively.
 
 ip_local_reserved_ports - list of comma separated ranges
@@ -943,8 +944,8 @@ ip_unprivileged_port_start - INTEGER
 	This is a per-namespace sysctl.  It defines the first
 	unprivileged port in the network namespace.  Privileged ports
 	require root or CAP_NET_BIND_SERVICE in order to bind to them.
-	To disable all privileged ports, set this to 0.  It may not
-	overlap with the ip_local_reserved_ports range.
+	To disable all privileged ports, set this to 0.  They must not
+	overlap with the ip_local_port_range.
 
 	Default: 1024
 
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH v2] net: Fix a documentation bug wrt. ip_unprivileged_port_start

[David Miller]

From: Maciej Żenczykowski <zenczykowski@gmail.com>
Date: Mon, 25 Nov 2019 14:48:00 -0800

> From: Maciej Żenczykowski <maze@google.com>
> 
> It cannot overlap with the local port range - ie. with autobind selectable
> ports - and not with reserved ports.
> 
> Indeed 'ip_local_reserved_ports' isn't even a range, it's a (by default
> empty) set.
> 
> Fixes: 4548b683b781 ("Introduce a sysctl that modifies the value of PROT_SOCK.")
> Signed-off-by: Maciej Żenczykowski <maze@google.com>

Applied.