Bug 1460217

For WWAN types (among others), there is a distinction between the "ifindex" and the "ip_ifindex". Yes, we don't properly expose the MTU in this case. Should be improved.

It was also discussed, whether these device types shoudn't be split. Currently, one NMDevice in a way represents two interfaces. Instead, there should be a "controller device" and an "ip device". That would require a larger work.

Created attachment 1355850 [details]
Patch: got mtu from ip device

(In reply to Thomas Haller from comment #1)
> For WWAN types (among others), there is a distinction between the "ifindex"
> and the "ip_ifindex". Yes, we don't properly expose the MTU in this case.
> Should be improved.
> 
> It was also discussed, whether these device types shoudn't be split.
> Currently, one NMDevice in a way represents two interfaces. Instead, there
> should be a "controller device" and an "ip device". That would require a
> larger work.

Yeah, probably a major rework is something we want to do here.
Anyway, fixing the MTU in this case seems as easy as getting it from the platform events reported on the "ip_ifindex".
If there would not be a case where we get 2 different MTUs from the "controller device" and the associated "ip device" it should be safe and good enough.

Please comment attached patch

(In reply to Francesco Giudici from comment #2)
> Created attachment 1355850 [details]
> Patch: got mtu from ip device

A subsequent device_link_changed() would reset it to something else and two interface would fight for setting the MTU property. At least, if ip-ifindex is set, then the MTU from the non-ip-ifindex must be ignored.

But may one day we might want to split the IP-device. At that point, it might be  be better if "mtu" references the MTU of the device with the non-ip-ifindex. You change that meaning now (which is a change in behavior), but later, we may not want to revert the behavior again.

I am not convinced that this is the right approach. Dunno.

(In reply to Francesco Giudici from comment #3)
> (In reply to Thomas Haller from comment #1)
> > For WWAN types (among others), there is a distinction between the "ifindex"
> > and the "ip_ifindex". Yes, we don't properly expose the MTU in this case.
> > Should be improved.
> > 
> > It was also discussed, whether these device types shoudn't be split.
> > Currently, one NMDevice in a way represents two interfaces. Instead, there
> > should be a "controller device" and an "ip device". That would require a
> > larger work.
> 
> Yeah, probably a major rework is something we want to do here.
> Anyway, fixing the MTU in this case seems as easy as getting it from the
> platform events reported on the "ip_ifindex".
> If there would not be a case where we get 2 different MTUs from the
> "controller device" and the associated "ip device" it should be safe and
> good enough.

I think the iface and ip_iface can have different MTUs and then NM would report a a random one between them. In my opinion, it's better to fix it in other ways, as considering the the devices as distinct, or adding a new property for the IP interface MTU.

(In reply to Beniamino Galvani from comment #5)
> (In reply to Francesco Giudici from comment #3)
> > (In reply to Thomas Haller from comment #1)
> > > For WWAN types (among others), there is a distinction between the "ifindex"
> > > and the "ip_ifindex". Yes, we don't properly expose the MTU in this case.
> > > Should be improved.
> > > 
> > > It was also discussed, whether these device types shoudn't be split.
> > > Currently, one NMDevice in a way represents two interfaces. Instead, there
> > > should be a "controller device" and an "ip device". That would require a
> > > larger work.
> > 
> > Yeah, probably a major rework is something we want to do here.
> > Anyway, fixing the MTU in this case seems as easy as getting it from the
> > platform events reported on the "ip_ifindex".
> > If there would not be a case where we get 2 different MTUs from the
> > "controller device" and the associated "ip device" it should be safe and
> > good enough.
> 
> I think the iface and ip_iface can have different MTUs and then NM would
> report a a random one between them. In my opinion, it's better to fix it in
> other ways, as considering the the devices as distinct, or adding a new
> property for the IP interface MTU.

In this specific case the cdc-wdm0 device is a character device speaking qmi to control the modem: the exposed network device is "wwp0s20u" and is the only one for which network configuration (ip conf, mtu, hw address...) apply.
I cannot figure out a case where we have a MTU on both device and ip_iface and they differs... because at the end we would have only one network configuration, and so only one correct MTU, isn't it? And in this case, wouldn't be correct to report the ip_interface MTU (as it embodies the ip connection we are tracking)?

In order to be safe in the event that the control interface has a valid mtu we have two choices:
1) take the MTU only from the ip_interface when present (as Thomas suggested)
2) take the MTU from the ip_interface only if the main device as MTU unset: this would require to track with a gboolean if the MTU has been set from the ip_interface or the main one.

I wonder if it is worth to spend an effort to do this or just let it as is as we will soon or later do a major rework there.
Well, the effort would be so tiny and the result immediate that maybe we can do this: option 1) seems to me the right thing to do, option 2) is instead a safe approach to have mtu on this kind of devices in the case it is not set at all.

> I cannot figure out a case where we have a MTU on both device and ip_iface and 
> they differs...

if they would never differ, the patch would not be useful.

(In reply to Thomas Haller from comment #7)
> > I cannot figure out a case where we have a MTU on both device and ip_iface and 
> > they differs...
> 
> if they would never differ, the patch would not be useful.

Right! Sorry, I meant, both set to a valid value and differ... I would expect the control interface to never have a valid MTU.

Created attachment 1358078 [details]
Patch: update device mtu from ip interface

I realized that this simple case, i.e., a device tracking a control device (which is a char device with no network interface), it is easily detectable checking the ifindex. If it is 0, we know that we are in this case and that the only exposed network interface (and so MTU) is from the ip interface.

Please, check the updated patch.

  if (!priv->ifindex && pllink->mtu)

the correct way for checking for valid ifindex is "ifindex > 0". At various places we set ifindex to -1 to indicate an invalid ifindex. We shouldn't do that, and maybe there exist no more code paths that still do that. But unless you proof that (and make sure that it continues to be true in future), checks must consider that there might be negative values.

(In reply to Thomas Haller from comment #10)
>   if (!priv->ifindex && pllink->mtu)
> 
> the correct way for checking for valid ifindex is "ifindex > 0". At various
> places we set ifindex to -1 to indicate an invalid ifindex. We shouldn't do
> that, and maybe there exist no more code paths that still do that. But
> unless you proof that (and make sure that it continues to be true in
> future), checks must consider that there might be negative values.

I missed it, thanks.
Changed the condition to check for invalid ifindex:
priv->ifindex <= 0

Created attachment 1358664 [details]
Patch: update device mtu from ip interface

I think it's correct to export the MTU of IP interface if there is no lower interface. Patch lgtm.

Merged on master and nm-1-10 branch:
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=efed5254cde007efcb48e353bd00651f37555e6d
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9c634e13c336804f946b595a8de3eb7e69f85df1

working well, covered in CI

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2018:0778