bpf: fix setting read timeout on ppc64

[tuexen]

Summary

On platforms other than amd64, BIOCSRTIMEOUT is equal to BIOCSRTIMEOUT32. Therefore, running the COMPAT_FREEBSD32 code basically clears tv_usec on big endian platforms. When tcpdump is used, the timeout requested is 100ms, which gets cleared to 0 on ppc64 platforms. This results in tcpdump showing the packets only when the read buffer is full.

This bug was reported by ivy.

Test Plan

Run tcpdump on a ppc64 platform and observe traffic with a delay of about 100ms.

[adrian]

good catch! would you mind adding a note in the code about this so it's not accidentally removed in the future when someone tinkers with compat stuff?

[tuexen]

Add comment as suggested by Adrian.

[ivy]

is it helpful for me to test this? it's a bit awkward here due to the state of my VMs but i can do.

[kib]

No, this is not a right fix.

The code should use SV_CURPROC_FLAG(SV_ILP32)) to differentiate between 32bit and host-native process calling the ioctl, then it can decide if this is BIOCSRTIMEOUT or BIOCSRTIMEOUT32. It should not compile conditionally based on arch at all, including the line 1422.

[tuexen]

In D56399#1291025, @ivy wrote:

is it helpful for me to test this? it's a bit awkward here due to the state of my VMs but i can do.

I tested it on my machine and it fixes the issue for me. Just wanted to let you know that. I am pretty sure that this is the root cause of the problem with tcpdump you experienced. So there os no need for you to test it if your VMs are in a suboptimal state.

[kib]

sys/net/bpf.c

1445

These two lines should be changed to use the TV_CP() macro from sys/abi_compat.h

[tuexen]

In D56399#1291026, @kib wrote:

No, this is not a right fix.

The code should use SV_CURPROC_FLAG(SV_ILP32)) to differentiate between 32bit and host-native process calling the ioctl, then it can decide if this is BIOCSRTIMEOUT or BIOCSRTIMEOUT32. It should not compile conditionally based on arch at all, including the line 1422.

OK. Doesn't this comment apply to all places in this file using #if defined(COMPAT_FREEBSD32) && defined(__amd64__)?

[kib]

In D56399#1291029, @tuexen wrote:

In D56399#1291026, @kib wrote:

No, this is not a right fix.

The code should use SV_CURPROC_FLAG(SV_ILP32)) to differentiate between 32bit and host-native process calling the ioctl, then it can decide if this is BIOCSRTIMEOUT or BIOCSRTIMEOUT32. It should not compile conditionally based on arch at all, including the line 1422.

OK. Doesn't this comment apply to all places in this file using #if defined(COMPAT_FREEBSD32) && defined(__amd64__)?

Seems so. They are all about TIMEOUTs so seemingly have the same problem of mixing native and compat32 ioctl commands.

[tuexen]

In D56399#1291030, @kib wrote:

In D56399#1291029, @tuexen wrote:

In D56399#1291026, @kib wrote:

No, this is not a right fix.

The code should use SV_CURPROC_FLAG(SV_ILP32)) to differentiate between 32bit and host-native process calling the ioctl, then it can decide if this is BIOCSRTIMEOUT or BIOCSRTIMEOUT32. It should not compile conditionally based on arch at all, including the line 1422.

OK. Doesn't this comment apply to all places in this file using #if defined(COMPAT_FREEBSD32) && defined(__amd64__)?

Seems so. They are all about TIMEOUTs so seemingly have the same problem of mixing native and compat32 ioctl commands.

OK. Will update the patch and test it. But my test machine panics now right after booting when running ntp. Need to work around that first.

[glebius]

But my test machine panics now right after booting when running ntp. Need to work around that first.

Just pushed the fix.

[tuexen]

Partially address comments from kib.

[tuexen]

sys/net/bpf.c

1445

I don't know how to do it. The definition of TV_CP is:

#define	TV_CP(src, dst, fld) do {		\
	CP((src).fld, (dst).fld, tv_sec);	\
	CP((src).fld, (dst).fld, tv_usec);	\
} while (0)

Don't we need two structs (src and dst), where the structure timeval are components of with the name fld? I think this does not apply here.

[tuexen]

In D56399#1291031, @tuexen wrote:

In D56399#1291030, @kib wrote:

In D56399#1291029, @tuexen wrote:

In D56399#1291026, @kib wrote:

No, this is not a right fix.

The code should use SV_CURPROC_FLAG(SV_ILP32)) to differentiate between 32bit and host-native process calling the ioctl, then it can decide if this is BIOCSRTIMEOUT or BIOCSRTIMEOUT32. It should not compile conditionally based on arch at all, including the line 1422.

How can this be done? BIOCSRTIMEOUT and BIOCSRTIMEOUT32 are the same on all platforms except amd64. switch does not allow to have multiple identical case values.
I don't like the asymmetry we now have between:

#if defined(COMPAT_FREEBSD32) && defined(__amd64__)
	case BIOCSRTIMEOUT32:
#endif

and

#ifdef COMPAT_FREEBSD32
			struct timeval32 *tv32;
			struct timeval tv64;
...
#endif

but this is the closed I could get to your suggestion.

OK. Doesn't this comment apply to all places in this file using #if defined(COMPAT_FREEBSD32) && defined(__amd64__)?

Seems so. They are all about TIMEOUTs so seemingly have the same problem of mixing native and compat32 ioctl commands.

OK. Will update the patch and test it. But my test machine panics now right after booting when running ntp. Need to work around that first.

[kib]

In D56399#1291090, @tuexen wrote:

In D56399#1291031, @tuexen wrote:

In D56399#1291030, @kib wrote:

In D56399#1291029, @tuexen wrote:

In D56399#1291026, @kib wrote:

No, this is not a right fix.

The code should use SV_CURPROC_FLAG(SV_ILP32)) to differentiate between 32bit and host-native process calling the ioctl, then it can decide if this is BIOCSRTIMEOUT or BIOCSRTIMEOUT32. It should not compile conditionally based on arch at all, including the line 1422.

How can this be done? BIOCSRTIMEOUT and BIOCSRTIMEOUT32 are the same on all platforms except amd64. switch does not allow to have multiple identical case values.
I don't like the asymmetry we now have between:

#if defined(COMPAT_FREEBSD32) && defined(__amd64__)
	case BIOCSRTIMEOUT32:
#endif

and

#ifdef COMPAT_FREEBSD32
			struct timeval32 *tv32;
			struct timeval tv64;
...
#endif

but this is the closed I could get to your suggestion.

OK. Doesn't this comment apply to all places in this file using #if defined(COMPAT_FREEBSD32) && defined(__amd64__)?

Seems so. They are all about TIMEOUTs so seemingly have the same problem of mixing native and compat32 ioctl commands.

OK. Will update the patch and test it. But my test machine panics now right after booting when running ntp. Need to work around that first.

You can put something like this outside the switch:

#if defined(COMPAT_FREEBSD32)
	if (SV_CURPROC_FLAG(SV_ILP32) && cmd == BIOCSRTIMEOUT32)
            cmd = BIOCSRTIMEOUT;
#endif

and then remove BIOCSRTIMEOUT32 case from the switches at all.

sys/net/bpf.c

1445

TV_CP(*tv32, *tv) ?

[tuexen]

As kib suggested.

sys/net/bpf.c

1204

Should this SV_PROC_FLAG() also be SV_CURPROC_FLAG?

1445

/usr/home/tuexen/freebsd-src/sys/net/bpf.c:1442:21: error: too few arguments provided to function-like macro invocation
 1442 |                                 TV_CP(*tv32, *tv);
      |                                                 ^
/usr/home/tuexen/freebsd-src/sys/sys/abi_compat.h:57:9: note: macro 'TV_CP' defined here
   57 | #define TV_CP(src, dst, fld) do {               \
      |         ^
/usr/home/tuexen/freebsd-src/sys/net/bpf.c:1442:5: error: use of undeclared identifier 'TV_CP'
 1442 |                                 TV_CP(*tv32, *tv);
      |                                 ^

The macro has three arguments. We are missing the fld.

[kib]

sys/net/bpf.c

1204

Does not matter, the outcome must be the same. You might unify it with your changes.

1445

I see, I was wrong there. It is still more proper to use the CP() macro for struct timeval fields copy.
I will add something for convenient copy of timeval vs timeval32 standalone after your change goes in.

[tuexen]

Improve consistency.

[kib]

sys/net/bpf.c

1212

A comment explaining why this is needed would be helpful, I think you already have one in the previous version of the patch.

[tuexen]

Add comment as suggested.

[tuexen]

Fix typo.

[kib]

sys/net/bpf.c

1217

I would remove the second sentence.