Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NetworkInterface.Linux: take into account physical link status for OperationalStatus and GetIsNetworkAvailable #44867

Merged
merged 1 commit into from
Nov 26, 2020
Merged

NetworkInterface.Linux: take into account physical link status for OperationalStatus and GetIsNetworkAvailable #44867

merged 1 commit into from
Nov 26, 2020

Conversation

tmds
Copy link
Member

@tmds tmds commented Nov 18, 2020

Fixes #30581

@wfurt ptal

@ghost
Copy link

ghost commented Nov 18, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details
Description:

Fixes #30581

@wfurt ptal
Author: tmds
Assignees: -
Labels:

area-System.Net
Milestone: -

stephentoub
stephentoub reviewed Nov 18, 2020
View reviewed changes
src/libraries/Native/Unix/System.Native/pal_networkchange.c
@@ -111,9 +108,6 @@ void SystemNative_ReadEvents(int32_t sock, NetworkChangeEvent onNetworkChange)
case RTM_NEWLINK:
onNetworkChange(sock, ReadNewLinkMessage(hdr));
break;
case RTM_DELLINK:
onNetworkChange(sock, LinkRemoved);
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is handling for RTM_DELLINK no longer needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar LinkRemoved was not handled on managed side, so I removed it here also.

stephentoub
stephentoub reviewed Nov 18, 2020
View reviewed changes
src/libraries/Native/Unix/System.Native/pal_networkchange.c
@@ -68,12 +68,9 @@ static NetworkChangeKind ReadNewLinkMessage(struct nlmsghdr* hdr)
assert(hdr != NULL);
struct ifinfomsg* ifimsg;
ifimsg = (struct ifinfomsg*)NLMSG_DATA(hdr);
if (ifimsg->ifi_family == AF_INET)
if (ifimsg->ifi_family == AF_UNSPEC)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for AF_INET was simply wrong before?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and on the managed side LinkAdded was ignored. So there is no possibility of regression here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess that in the past presence of IPv4 address was considered as "available". Perhaps pre-dates IPv6

wfurt
wfurt reviewed Nov 19, 2020
View reviewed changes
src/libraries/Native/Unix/System.Native/pal_interfaceaddresses.c
nii->OperationalState = (ifaddrsEntry->ifa_flags & IFF_UP) ? OperationalStatus_Up : OperationalStatus_Down;
// OperationalState returns whether the interface can transmit data packets.
// The administrator must have enabled the interface (IFF_UP), and the cable must be plugged in (IFF_RUNNING).
nii->OperationalState = ((ifaddrsEntry->ifa_flags & (IFF_UP|IFF_RUNNING)) == (IFF_UP|IFF_RUNNING)) ? OperationalStatus_Up : OperationalStatus_Down;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the IFF_RUNNING reliable? e.g. would it also work for bridges, VPNs and interface types other than ethernet?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I think so because they have the UP and RUNNING flags set on my system:

enp37s0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
lo: flags=73<UP,LOOPBACK,RUNNING>  mtu 65536
tun0: flags=4305<UP,POINTOPOINT,RUNNING,NOARP,MULTICAST>  mtu 1360
virbr0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
vnet0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be. for me it looks like:

br-9a8c3c79be53: flags=4099<UP,BROADCAST,MULTICAST>  mtu 1500
        inet 172.18.0.1  netmask 255.255.0.0  broadcast 172.18.255.255
        inet6 fe80::42:9cff:fe2a:1051  prefixlen 64  scopeid 0x20<link>
        ether 02:42:9c:2a:10:51  txqueuelen 0  (Ethernet)
        RX packets 92528  bytes 17957517 (17.9 MB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 117337  bytes 121598151 (121.5 MB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

br-9ee3891a5abb: flags=4099<UP,BROADCAST,MULTICAST>  mtu 1500
        inet 172.19.0.1  netmask 255.255.0.0  broadcast 172.19.255.255
        inet6 fe80::42:17ff:fec0:867b  prefixlen 64  scopeid 0x20<link>
        ether 02:42:17:c0:86:7b  txqueuelen 0  (Ethernet)
        RX packets 118  bytes 19808 (19.8 KB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 104  bytes 69596 (69.5 KB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

docker0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
        inet 172.17.0.1  netmask 255.255.0.0  broadcast 172.17.255.255
        inet6 fe80::42:daff:fed5:e06e  prefixlen 64  scopeid 0x20<link>
        inet6 fc00::1  prefixlen 64  scopeid 0x0<global>
        inet6 fe80::1  prefixlen 64  scopeid 0x20<link>
        ether 02:42:da:d5:e0:6e  txqueuelen 0  (Ethernet)
        RX packets 2403844  bytes 145172137 (145.1 MB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 3343620  bytes 18861164993 (18.8 GB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0**

but I don't know how to reliably tell if the bride is up or now.

Also for you if tun0 and virbr0 is up & running. can you really send traffic to it?
I think I'm ok claiming they are up if Linux claim them as such regardless of that the actual ability to communicate. That still seems like improvement over just using IFF_UP.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for you if tun0 and virbr0 is up & running. can you really send traffic to it?

Using RUNNING for virbr0 seems appropriate. When I shut down all VMs, status for virbr0 changes to:

virbr0: flags=4099<UP,BROADCAST,MULTICAST>  mtu 1500

@tmds
Copy link
Member Author

tmds commented Nov 23, 2020

@wfurt @stephentoub this is ready to be merged.

@antonfirsov
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt
Copy link
Member

wfurt commented Nov 23, 2020

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt
Copy link
Member

wfurt commented Nov 23, 2020

outer loop impacted by #45061

@wfurt wfurt merged commit 31ffad2 into dotnet:master Nov 26, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 26, 2020
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wfurt wfurt wfurt left review comments

@stephentoub stephentoub stephentoub approved these changes

Assignees
No one assigned
Labels
area-System.Net
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

NetworkChange.NetworkAvailabilityChanged does not detect physical link change on linux
6 participants
@tmds @antonfirsov @wfurt @stephentoub @karelz @Dotnet-GitSync-Bot