Page MenuHomeFreeBSD

pf: Introduce nvlist variant of DIOCADDRULE
ClosedPublic

Authored by kp on Apr 2 2021, 6:54 PM.

Details

Summary

This will make future extensions of the API much easier.
The intent is to remove support for DIOCADDRULE in FreeBSD 14.

MFC after: 4 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 38279
Build 35168: arc lint + arc unit

Event Timeline

markj added inline comments.
sys/netpfil/pf/pf_ioctl.c
95

Why not include it via a system path?

2347

malloc(M_WAITOK) won't fail.

2353

Do nvl and nvlpacked get freed anywhere?

  • Free nvl/nvlpacked
  • Don't check for failure on M_WAITOK

Seems ok to me for what it's worth. My comments are mostly nits.

sys/netpfil/pf/pf_ioctl.c
1631

They're equivalent, but it looks like the last parameter should be sizeof(*paddr).

1729

I think there's nothing checking that the supplied array has exactly two elements. Should the checking be more strict, so that an array of length 1 is rejected? There are a few examples of this.

sys/netpfil/pf/pf_nv.c
36

Same thing here with respect to the include path.

This revision is now accepted and ready to land.Apr 3 2021, 3:37 PM
  • Require the number of array elements to be exactly the maximum number
  • Explicitly check the nvlist error state
This revision now requires review to proceed.Apr 5 2021, 11:36 AM
glebius added inline comments.
sys/netpfil/pf/pf_ioctl.c
1698

Better use uint8_t in the new code.

1803

Don't we leak rule here?

2026

Better use uint32_t in new code. More of this applies to new files pf_nv.h and pf_nv.c

  • Use 'unit*' rather than 'u_int*'.
  • Fix potential rule leak
donner added inline comments.
sys/netpfil/pf/pf.h
192

Component "size" is unused. I do not see any use case for it. Can it be dropped?

kp marked an inline comment as done.Apr 7 2021, 7:01 PM
kp added inline comments.
sys/netpfil/pf/pf.h
192

It's not used here, but it is required for get operations (e.g. D29559). For those userspace must supply a larger buffer than required for just the request, so that the kernel can put the reply in the data buffer.

In that case len will be the length of the request data, but we will have provided size bytes of space for the kernel to put the response into.

kp marked an inline comment as done.

Rebase (remove rt_listid)

This revision was not accepted when it landed; it landed in state Needs Review.Apr 10 2021, 9:17 AM
This revision was automatically updated to reflect the committed changes.