Make sbin/ipfw on RELENG_14 compatible to ipfw kernel module on RELENG_15+
AcceptedPublic
Actions

Authored by lytboris_gmail.com on Sun, Jan 18, 10:31 AM.

Details

Reviewers

ae
melifaro
glebius
jhb
cperciva

Group Reviewers

releng

Summary

As for now there is no compatibility layer for a changed KABI of ipfw kernel module after D46183 was merged. This leads to inability to upgrade a remote server running ipfw as RELENG_15 kernel does not accept commands from an old ipfw binary.

Instead of making a kernel-side compatibility layer that is hard to debug and maintain, we can import sbin/ipfw from RELENG_15 as a new binary sbin/ipfw15 into RELENG_14. Once new binary is there,
original sbin/ipfw can detect new KABI and run the new binary instead.

A trivial and naive patch for sbin/ipfw is attached. Header-only modified copy of sbin/ipfw from RELENG_15 is attached to the diff as well.

Test Plan

Install RELENG_14 on a VM and put a kernel from RELENG_15 into it.
Build and install both sbin/ipfw15 and sbin/ipfw from sources
Try to add/remove rules, etc

Running the patched ipfw on RELENG_14 will produce:

r14# ipfw show
WARNING! KBI incompatibility for ipfw is detected, trying to run /sbin/ipfw15.
01000 12844 2993997 allow ip from any to any
65535     0       0 count ip from any to any not // orphaned dynamic states counter
65535    17    3208 deny ip from any to any
r14#

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

lytboris_gmail.com created this revision.Sun, Jan 18, 10:31 AM

Herald added subscribers: ziaee, donner, imp. · View Herald TranscriptSun, Jan 18, 10:31 AM

lytboris_gmail.com requested review of this revision.Sun, Jan 18, 10:31 AM

glebius added inline comments.Mon, Jan 19, 6:45 PM

sbin/ipfw/main.c
694	What's the point of using __FreeBSD_version here? It will be substituted by pre-processor at compilation time. We do not expect this code to be ever compiled outside of stable/14 and its descendants.
702
704	I can't imagine any exploitable use here, except a stupid admin who has "." in their PATH. However, just for the safety I would suggest two things: Add two static strings "/sbin/ipfw15" and "/sbin/dnctl15", so that you don't need to use malloc and snprintf. use `execv(g_co.prog == cmdline_prog_ipfw ? ipfw15_string : dnctl15_string, av)` instead of `execvp`.

lytboris_gmail.com updated this revision to Diff 170092.Tue, Jan 20, 6:01 AM

lytboris_gmail.com edited the summary of this revision. (Show Details)

lytboris_gmail.com edited the test plan for this revision. (Show Details)

lytboris_gmail.com marked 3 inline comments as done.Tue, Jan 20, 6:08 AM

lytboris_gmail.com updated this revision to Diff 170093.Tue, Jan 20, 6:11 AM

glebius added inline comments.Tue, Jan 20, 5:40 PM

sbin/ipfw/main.c
727–736	I don't think these extra checks with stat(2) add any value. The execution attempt will do the same checks internally. You may add an extra printf() with strerror() in the case of execv() failed.

Removed stat() checks, added execv() error reporting

To me the patch looks good enough. I can't approve the approach taken, that adds a future version tool to a stable/xx tree. This is up to releng as the main upgrade path gatekeeper and @jhb who requested the compatibility shim.

My personal opinion on this approach is positive, because it:

adds compat shim to the old branch, instead of polluting the actively developed branch.
makes the shim very non-intrusive to the non-compatibility code, which minimizes chances for a regression.