gre: use its own internal flag
AbandonedPublic
Actions

Authored by pouria on Sun, Dec 28, 9:51 PM.

Details

Reviewers

glebius
zlei
bz

Group Reviewers

network

Summary

Replaced the IFF_DRV_RUNNING with GRE_FLAG_RUNNING and added
gre_flags in gre_softc so gre can use internal flags.
I want to remove all shared uses of the IFF_DRV_RUNNING flag from
interface drivers and this is the first revision.
The idea came to me by @glebius and @zlei comments in geneve implementation.
Please tell me to stop right there if the approach is wrong.

Besides IFF_DRV_RUNNING, any future efforts to implement MGRE and NVGRE will
require internal flags in the gre_softc.
Therefore, gre_options is not appropriate for those changes, given its name
and the in[6]_gre_setopts functions, which take their flags directly from userspace.

P.S: GRE_FLAG_* macro is similar to GRE_FLAGS_* used by grehdr.
After some thought, I concluded the use of GRE_FLAG_* name is fine because of
similar usages in other drivers.
IMHO, the real issue is GRE_FLAGS_* names in grehdr which should include
an HDR prefix or suffix to distinguish it from softc flags. so, if you
think it's not good. tell me to change it.

Test Plan

if_gre currently has no tests. I'll add them in a separate revision.
However, this change is tested. There is a special case for spoofed
source addresses in gre that was previously handled by IFF_DRV_RUNNING.
In that case, the driver should detect it by returing ENETDOWN and it does with
this patch.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 69493
Build 66376: arc lint + arc unit

Event Timeline

pouria created this revision.Sun, Dec 28, 9:51 PM

Herald added subscribers: melifaro, ae, imp. · View Herald TranscriptSun, Dec 28, 9:51 PM

pouria requested review of this revision.Sun, Dec 28, 9:51 PM

Harbormaster completed remote builds in B69493: Diff 168669.Sun, Dec 28, 9:52 PM

bz requested changes to this revision.Sun, Dec 28, 11:10 PM

bz added a subscriber: bz.

bz added inline comments.

sys/net/if_gre.c
193	Doesn't this mean this will always report IFF_DRV_RUNNING now instead of conditional. Which is a bug. I do not know about the conversation with zlei and glebius but I see little value in moving the common flag into each driver for as long as we check the flag in common code. If we really wanted to do this the first thing we would need is an accessor function which each driver will implement so common code could query the state. One would implement this on top of IFF_DRV_RUNNING as-is most likely first and then only in a second pass remove all the "public" versions with a driver-private flag. Unless we want to get rid of if_drv_flags (which also would mean migrating OACTIVE) I see very little value of doing any of this. So I would wait until further clarifications from zlei or glebius about the actual goal.

This revision now requires changes to proceed.Sun, Dec 28, 11:10 PM

zlei added inline comments.Mon, Dec 29, 4:53 AM

sys/net/if_gre.c
193	The discussion is within https://reviews.freebsd.org/D54172 . @glebius suggested the driver flag `IFF_DRV_RUNNING`, A new properly written driver shall not rely on it as a measure to prevent the stack from talking to the driver. It appears there's little value to convert exiting drivers to use driver private flags, unless bugs are revealed to be related to `IFF_DRV_RUNNING`.

I wasn't sure whether this change was necessary or whether my approach was correct. I thought this was a known issue that someone would address eventually, and no one had enough time to do it. If that's not the case, I'll close this revision.
@bz Thank you for your feedback. @zlei If a bug appears in the future and the team decides it should be fixed, I can help.