Page MenuHomeFreeBSD
Differential D56399

bpf: fix setting read timeout on ppc64
ClosedPublic
Actions

Authored by tuexen on Tue, Apr 14, 9:00 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 21, 8:38 AM
Unknown Object (File)
Mon, Apr 20, 4:33 AM
Unknown Object (File)
Mon, Apr 20, 1:46 AM
Unknown Object (File)
Sat, Apr 18, 7:49 AM
Unknown Object (File)
Sat, Apr 18, 7:49 AM
Unknown Object (File)
Sat, Apr 18, 7:49 AM
Unknown Object (File)
Sat, Apr 18, 7:49 AM
Unknown Object (File)
Sat, Apr 18, 7:49 AM
View All 23 Files
Subscribers
ae
glebius
imp
melifaro

Details

Reviewers
ivy
adrian
kib
Commits
rG04b994d19eec: bpf: fix handling the read timeout on ppc64
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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

tuexen created this revision.Tue, Apr 14, 9:00 PM
Herald added subscribers: glebius, melifaro, ae, imp. · View Herald TranscriptTue, Apr 14, 9:00 PM
tuexen requested review of this revision.Tue, Apr 14, 9:00 PM
adrian added a comment.Tue, Apr 14, 9:02 PM

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 updated this revision to Diff 175528.Tue, Apr 14, 9:17 PM

Add comment as suggested by Adrian.

ivy added a comment.Tue, Apr 14, 9:19 PM

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 added a comment.Tue, Apr 14, 9:33 PM

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 added a comment.Tue, Apr 14, 9:35 PM
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 added inline comments.Tue, Apr 14, 9:38 PM
sys/net/bpf.c
1445

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

tuexen added a comment.Tue, Apr 14, 9:44 PM
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 added a comment.Tue, Apr 14, 9:49 PM
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 added a comment.Tue, Apr 14, 9:51 PM
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 added a comment.Tue, Apr 14, 11:24 PM

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

Just pushed the fix.

tuexen updated this revision to Diff 175543.Wed, Apr 15, 9:18 AM

Partially address comments from kib.

tuexen added inline comments.Wed, Apr 15, 9:23 AM
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 added a comment.Wed, Apr 15, 9:28 AM
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 added a comment.Wed, Apr 15, 9:46 AM
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 updated this revision to Diff 175564.Wed, Apr 15, 1:32 PM

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 added inline comments.Wed, Apr 15, 3:06 PM
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 updated this revision to Diff 175582.Wed, Apr 15, 4:03 PM

Improve consistency.

adrian accepted this revision.Wed, Apr 15, 4:04 PM
This revision is now accepted and ready to land.Wed, Apr 15, 4:04 PM
kib accepted this revision.Wed, Apr 15, 4:06 PM
kib added inline comments.
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 updated this revision to Diff 175587.Wed, Apr 15, 4:50 PM

Add comment as suggested.

This revision now requires review to proceed.Wed, Apr 15, 4:50 PM
tuexen updated this revision to Diff 175588.Wed, Apr 15, 4:54 PM

Fix typo.

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Apr 15, 7:28 PM
Closed by commit rG04b994d19eec: bpf: fix handling the read timeout on ppc64 (authored by tuexen). · Explain Why
This revision was automatically updated to reflect the committed changes.
tuexen added a commit: rG04b994d19eec: bpf: fix handling the read timeout on ppc64.
kib added inline comments.Wed, Apr 15, 11:11 PM
sys/net/bpf.c
1217

I would remove the second sentence.

Revision Contents
Changeset List

PathSize
sys/
net/
42 lines

Diff 175588

View Options

sys/net/bpf.c

Loading...