jail: Use flexible array member within struct prison_ip

[zlei]

Summary

Current implementation utilize off-by-one struct prison_ip to access the IPv[46] addresses. It is error prone and hence comes the regression fix D37732, D37871 and D37872 . Use flexible array member so that compiler will catch the errors and it is also easier to review.

No functional change intended.

[zlei]

This is a request by @melifaro in D37732 .

In D37732#858183, @melifaro wrote:

LGTM!
Do you think it would be possible to convert PR_IP to be static inline function, so that kind of erros are spotted by the compiler?

I'll take a look at that. Smart compilers help a lot ;)

[melifaro]

In D37874#860475, @zlei wrote:

This is a request by @melifaro in D37732 .

In D37732#858183, @melifaro wrote:

LGTM!
Do you think it would be possible to convert PR_IP to be static inline function, so that kind of erros are spotted by the compiler?

I'll take a look at that. Smart compilers help a lot ;)

Thank you!

sys/kern/kern_jail.c

578–582

Is this still needed?

627

‘pr_ip’ is an array. IIRC the typical pattern is either ‘arr’ or ‘&arr[0]’

[zlei]

sys/kern/kern_jail.c

578–582

Is this still needed?

Variable length arrays are allowed in ISO C99. They are standard and more elegant.

Zero-length arrays in struct / union is not allowed in ISO C but extensions of GCC (prior ISO C) and Clang.

I'd prefer ISO ones as maybe the extension (of zero-length arrays) will be disabled and ISO ones is enforced in future.

For now I'll remove it and leave a comment instead.

627

Good catch!
This is a middle night work and &arr is indeed wrong.

Well &arr point to the same address as arr or &arr[0], but it has different type.

&arr + 1 != arr + 1 .

[zlei]

Address @melifaro 's comment.

[zlei]

Reduce usage of parentheses .

[melifaro]

sys/kern/kern_jail.c

637

Nit: maybe that's more readable?

[zlei]

sys/kern/kern_jail.c

637

Yes I think so.

829–830

&ppip->pr_ip -> ppip->pr_ip

[zlei]

sys/kern/kern_jail.c

637

Nit: maybe that's more readable?

Create separate differential D37890 so it will be easy to review.

[melifaro]

sys/kern/kern_jail.c

2392

If pr->pr_addrs[PR_INET] is NULL (as the line below suggest) won't it crash?
Same for IPv6 case

[melifaro]

sys/kern/kern_jail.c

592

Maybe it's worth converting the macro to the function, so we get type checking & argument asserts for free?

[zlei]

sys/kern/kern_jail.c

2392

No, it won't crash.

Checked the implementation of vfs_setopt_part(struct vfsoptlist *opts, const char *name, void *value, int len), it internally calls bcopy(value, opt->value, len). As per BCOPY(3),

The bcopy() function copies len bytes from string src to string dst. The
two strings may overlap. If len is zero, no bytes are copied.

If pr->pr_addrs[PR_INET(6)] is null then len will be zero, and bcopy() will behave as a nop.

[zlei]

sys/kern/kern_jail.c

592

That make sense.

[melifaro]

sys/kern/kern_jail.c

2392

Mm. I meant that in order to call vfs_setopt_part(), the code has to resolve its arguments first.
It is possible for pr->pr_addrs[PR_INET] to be NULL.
My question was what happens with the pr->pr_addrs[PR_INET]->pr_ip4 argument

[zlei]

sys/kern/kern_jail.c

592

Maybe it's worth converting the macro to the function, so we get type checking & argument asserts for free?

Compiler complains (char *) to (const char*) converting. So I have to define two functions.

static const char *
pr_ip_get(const struct prison_ip *pip, const pr_family_t af, int idx)
{
	MPASS(pip);
	MPASS(af < PR_FAMILY_MAX);
	MPASS(idx >= 0);

	return (pip->pr_ip + pr_families[af].size * idx);
}

static char *
pr_ip_get_d(struct prison_ip *pip, const pr_family_t af, int idx)
{
	MPASS(pip);
	MPASS(af < PR_FAMILY_MAX);
	MPASS(idx >= 0);

	return (pip->pr_ip + pr_families[af].size * idx);
}

#define PR_IP(pip, i)	(pr_ip_get((pip), af, (i)))
#define PR_IPD(pip, i)	(pr_ip_get_d((pip), af, (i)))

That seems a bit of overkill .
@melifaro What do you think ?

[melifaro]

sys/kern/kern_jail.c

592

Well, I'd still prefer to have them.
In the prod version they'll effectively be the same one-liner, but we'll get a lot of validity checks.

If we had this before, the issue spotted in D37872 (

			case 0:
				bcopy(PR_IP(pr, i), PR_IPD(new, ips), size);

) would be caught by the compiler.

[zlei]

sys/kern/kern_jail.c

2392

Ooh, my fault.

For short no side effect.

Although pr->pr_addrs[PR_INET] might be NULL but we are not dereferencing NULL pointer.

It looks tricky but pr_ip4 is an (zero sized) array.
pr->pr_addrs[PR_INET]->pr_ip4 is equivalent to
&pr->pr_addrs[PR_INET]->pr_ip4[0] and
(struct in_addr *)((char *)(pr->pr_addrs[PR_INET]) + (sizeof(struct prison_ip) - sizeof(pr_ip[0])) .
That is the same with pr->pr_addrs[PR_INET] + 1 .

I'll demonstrate that online https://godbolt.org/z/f4hq63rdj

[melifaro]

sys/kern/kern_jail.c

2392

Yep. My bad. For some reason, the idea that pr_ip4 has a separate pointer was stuck in my head.
Thank you, no problem here. Thank you for the patience :-)

[zlei]

Define macros as functions

[zlei]

This should rebase onto D37906. I will commit and do that after D37906 is landed.

[glebius]

There are reasons why I avoid using the GCC extension (although supported by Clang) and used standard C. The reasons are spread around the Internet, but the best sum up of the reasoning can be found in the Linux kernel guidelines :)

https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays

So the GNU worlds itself gets rid of this GCC-ism. See Linux commit https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=5224f79096170bf7b92cc8fe42a12f44b91e5f62

Is it possible to fix the bugs without [0]?

[zlei]

In D37874#861264, @glebius wrote:

There are reasons why I avoid using the GCC extension (although supported by Clang) and used standard C. The reasons are spread around the Internet, but the best sum up of the reasoning can be found in the Linux kernel guidelines :)

https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays

So the GNU worlds itself gets rid of this GCC-ism. See Linux commit https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=5224f79096170bf7b92cc8fe42a12f44b91e5f62

IIRC Linus Torvalds decided to move the Linux kernel from C89 to modern C11 standard. I'm not familiar with Linux kernel code so I
can not tell which commit enforced that.

This should happen after the enforcement.

Is it possible to fix the bugs without [0]?

Indeed they're fixed without [0]. See D37871 and D37872.

I'd prefer the ISO standards but FreeBSD is not ready for it.

PS: Any plans FreeBSD move to C99 or C11 ?

[zlei]

In D37874#861283, @zlei wrote:

In D37874#861264, @glebius wrote:

There are reasons why I avoid using the GCC extension (although supported by Clang) and used standard C. The reasons are spread around the Internet, but the best sum up of the reasoning can be found in the Linux kernel guidelines :)

https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays

So the GNU worlds itself gets rid of this GCC-ism. See Linux commit https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=5224f79096170bf7b92cc8fe42a12f44b91e5f62

IIRC Linus Torvalds decided to move the Linux kernel from C89 to modern C11 standard. I'm not familiar with Linux kernel code so I
can not tell which commit enforced that.

This should happen after the enforcement.

Is it possible to fix the bugs without [0]?

Indeed they're fixed without [0]. See D37871 and D37872.

I'd prefer the ISO standards but FreeBSD is not ready for it.

PS: Any plans FreeBSD move to C99 or C11 ?

Ooh sorry for that. FreeBSD kernel source is build with C99.

Back to the problem,

Is it possible to fix the bugs without [0]?

Compiler complains if the members in union are flexible array,

/usr/src/sys/kern/kern_jail.c:580:8: error: flexible array member 'pr_ip' in a union is not allowed
                char pr_ip[];
                     ^
/usr/src/sys/kern/kern_jail.c:582:18: error: flexible array member 'pr_ip4' in a union is not allowed
                struct in_addr pr_ip4[];
                               ^
/usr/src/sys/kern/kern_jail.c:585:19: error: flexible array member 'pr_ip6' in a union is not allowed
                struct in6_addr pr_ip6[];

@glebius What about this one ?

struct prison_ip {
        struct epoch_context ctx;
        uint32_t        ips;
        union {
#ifdef INET
                struct in_addr pr_ip4[0];
#endif
#ifdef INET6
                struct in6_addr pr_ip6[0];
#endif
        };
        char pr_ip[];
};

That prevents problems of zero-length arrays and benefits variable-length arrays (flexible array) .

[glebius]

I think that zero-length arrays won't ever be admitted to the standard. However, I can imagine that a union of flexible ones (the code commented out) would be. That's why I don't want to add new code that heavily relies on zero-length arrays and rather prefer the ugly macros.

[zlei]

Address @glebius 's concern. Use flexible arrays.

[glebius]

Yes, this is better than what I did.

I think we need either name the function PR_IP() and skip macro, or save the name of the function as is, but convert usage to function name instead of macro name.

For the constness, I guess we can just drop the const version and have single non-const function. It was useful to catch errors at early stage of development of the code. Maybe not so useful today? What do you think?

[glebius]

Don't forget to change the title of the changeset when you commit.

[zlei]

Address @glebius 's feedback.

1. Remove macro PR_IPD
2. And drop some const qualifiers

[zlei]

Address @glebius 's comment, use function instead of macro PR_IP