jail: Fix wrong IPv[46] addresses inherited from parent jail

[zlei]

Summary

While creating jails with parameters ip4 or ip6 set to inherit, the jails's IPv4 or IPv6 addresses are not properly copied from parent.

Fixes: eb8dcdeac22d jail: network epoch protection for IP address lists

Test Plan

Run the following script:

#!/bin/sh

ifconfig lo0 inet 172.16.0.1/32 alias
ifconfig lo0 inet 172.16.0.2/32 alias
ifconfig lo0 inet6 2001:db8::1/128 alias
ifconfig lo0 inet6 2001:db8::2/128 alias

jail -c name=parent host.hostname=parent path=/ persist children.max=1 ip4.addr=172.16.0.1 ip4.addr=172.16.0.2 ip6.addr=2001:db8::1 ip6.addr=2001:db8::2
jexec parent /bin/sh -s stdin << EOF
  jail -c name=c1 host.hostname=c1 path=/ persist ip4=inherit ip6=inherit
  sleep 1
  jls -j c1
  jexec c1 ifconfig lo0
EOF

Verify the output:

   JID  IP Address      Hostname                      Path
     4  172.16.0.1      c1                            /
lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> metric 0 mtu 16384
	options=680003<RXCSUM,TXCSUM,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>
	inet6 2001:db8::1 prefixlen 128
	inet6 2001:db8::2 prefixlen 128
	inet 172.16.0.1 netmask 0xffffffff
	inet 172.16.0.2 netmask 0xffffffff
	groups: lo
	nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>

Could also verify via DDB show prison

[melifaro]

sys/kern/kern_jail.c

657

A comment elaborating why the first address is skipped will be helpful. Also: is it ips or ips - 1 ?

[zlei]

sys/kern/kern_jail.c

657

No, the first address is not skipped. prison_ip_dup() is going to duplicate all addresses of family af.

The design of struct prison has maximum two types of ip addresses.
IPv4 addresses is installed in prison->pr_addrs[PR_INET].
IPv6 addresses is installed in prison->pr_addrs[PR_INET6].
prison->pr_addrs[af] has type

struct prison_ip {
    struct epoch_context ctx;
    uint32_t	ips;
    union {
        struct in_addr pr_ip4[];
        struct in6_addr pr_ip6[];
    }
}

When allocation struct prison_ip additional space is preserved for the IPv[46] addresses ( struct in_addr [ips] or struct in6_addr[ips] ).
So to access the actual IPv[46] addresses you need off-by-one struct prison_ip, i.e.

struct prison_ip *pip;
struct in_addr *in = (struct in_addr  *)(pip + 1);
// or
struct in6_addr *in6 = (struct in6_addr  *)(pip + 1);

D37874 will shows that more clearly.

[zlei]

@melifaro @kp
I'd like to combine this (D37871) with D37872 , as they're simple and are same type of mistakes,
although cause different symptoms.

How shall I do ? Commit with two Differential Revision , or combine them into a new differential ?

[melifaro]

In D37871#861097, @zlei wrote:

@melifaro @kp
I'd like to combine this (D37871) with D37872 , as they're simple and are same type of mistakes,
although cause different symptoms.

How shall I do ? Commit with two Differential Revision , or combine them into a new differential ?

I'd commit both separately, as it's easier to track and to reason about.

[glebius]

The problem isn't in the beginning and end of bcopy, it is in the size, which doesn't account for the header structure itself. The code should be:

bcopy(ppr->pr_addrs[af], pr->pr_addrs[af],
    sizeof(struct prison_ip) + pr->pr_addrs[af]->ips * pr_families[af].size);

Thanks for finding this problem. I guess it was a hard one to nail down.

[glebius]

On the second thought, your code is better than mine since we don't want to intentionally overwrite the header which was already correctly initialized by prison_ip_alloc() (although we overwrite with the same values). Given that you also find several instances of this mistake, I'd suggest to go around all code that does copying and substitute: (pip + 1) to PR_IP(pip, 0). What do you think?

[glebius]

I meant: first fix all code that uses pip, but must be pip + 1 into PR_IP(pip, 0), and then substitute that is already correct (pip + 1) into PR_IP(pip, 0).

[glebius]

I didn't mean that to be a requirement, it is a suggestion :)

[kp]

Approved by: kp (mentor)

[zlei]

In D37871#863168, @glebius wrote:

I meant: first fix all code that uses pip, but must be pip + 1 into PR_IP(pip, 0), and then substitute that is already correct (pip + 1) into PR_IP(pip, 0).

PR_IP(pip, 0) sounds copying the first one only, actually we are copying arrays of addresses.

I'd prefer D37874 rather than pip + 1 or PR_IP(pip, 0) .

[glebius]

In D37871#863486, @zlei wrote:

I'd prefer D37874 rather than pip + 1 or PR_IP(pip, 0) .

Agreed