Page MenuHomeFreeBSD

jail: Use flexible array member within struct prison_ip
ClosedPublic

Authored by zlei on Dec 25 2022, 6:59 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 24 2024, 2:23 AM
Unknown Object (File)
Feb 22 2024, 10:51 AM
Unknown Object (File)
Feb 22 2024, 10:51 AM
Unknown Object (File)
Feb 22 2024, 10:42 AM
Unknown Object (File)
Feb 22 2024, 10:42 AM
Unknown Object (File)
Feb 22 2024, 10:42 AM
Unknown Object (File)
Feb 22 2024, 10:42 AM
Unknown Object (File)
Feb 22 2024, 10:42 AM
Subscribers

Details

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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This is a request by @melifaro in D37732 .

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 ;)

In D37874#860475, @zlei wrote:

This is a request by @melifaro in D37732 .

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]’

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 marked 2 inline comments as done.Dec 27 2022, 3:42 AM

Reduce usage of parentheses .

melifaro added inline comments.
sys/kern/kern_jail.c
637

Nit: maybe that's more readable?

This revision is now accepted and ready to land.Dec 27 2022, 8:40 PM
sys/kern/kern_jail.c
637

Yes I think so.

829–830

&ppip->pr_ip -> ppip->pr_ip

This revision now requires review to proceed.Dec 28 2022, 2:57 AM
zlei marked an inline comment as done.Dec 28 2022, 4:00 AM
zlei added inline comments.
sys/kern/kern_jail.c
637

Nit: maybe that's more readable?

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

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

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 marked an inline comment as done.Dec 29 2022, 3:43 PM
zlei added inline comments.
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 marked an inline comment as done.Dec 29 2022, 4:11 PM
zlei added inline comments.
sys/kern/kern_jail.c
592

That make sense.

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

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 ?

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 marked an inline comment as done.Dec 29 2022, 5:51 PM
zlei added inline comments.
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

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 marked an inline comment as done.

Define macros as functions

zlei marked 3 inline comments as done.Dec 29 2022, 5:59 PM
This revision is now accepted and ready to land.Dec 29 2022, 6:00 PM

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

glebius requested changes to this revision.Dec 30 2022, 5:02 AM

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]?

This revision now requires changes to proceed.Dec 30 2022, 5:02 AM

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 ?

In D37874#861283, @zlei 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) .

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.

Address @glebius 's concern. Use flexible arrays.

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?

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

This revision is now accepted and ready to land.Jan 10 2023, 7:45 PM

Address @glebius 's feedback.

  1. Remove macro PR_IPD
  2. And drop some const qualifiers
This revision now requires review to proceed.Feb 21 2023, 2:42 PM

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

zlei retitled this revision from jail: Use union of zero length arrays in struct prison_ip to jail: Use flexible array member within struct prison_ip.Feb 21 2023, 3:26 PM
zlei edited the summary of this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Feb 28 2023, 10:21 AM
This revision was automatically updated to reflect the committed changes.