[bpf,3/5] netkit: Fix syncing peer device mtu with primary

Commit Message

Daniel Borkmann May 24, 2024, 1:01 p.m. UTC

Implement the ndo_change_mtu callback in netkit in order to align the MTU
to the primary device. This is needed in order to sync MTUs to the latter
from the control plane (e.g. Cilium) which does not have access into the
Pod's netns.

Fixes: 35dfaad7188c ("netkit, bpf: Add bpf programmable net device")
Signed-off-by: Daniel Borkmann <[e-mail]>
Cc: Joe Stringer <[e-mail]>
---
 drivers/net/netkit.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Nikolay Aleksandrov May 24, 2024, 2:15 p.m. UTC | #1

On 5/24/24 16:01, Daniel Borkmann wrote:
> Implement the ndo_change_mtu callback in netkit in order to align the MTU
> to the primary device. This is needed in order to sync MTUs to the latter
> from the control plane (e.g. Cilium) which does not have access into the
> Pod's netns.
> 
> Fixes: 35dfaad7188c ("netkit, bpf: Add bpf programmable net device")
> Signed-off-by: Daniel Borkmann <[e-mail]>
> Cc: Joe Stringer <[e-mail]>
> ---
>  drivers/net/netkit.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 

This one has unexpected behaviour IMO. If the app sets the MTU and we
silently overwrite, then it may continue working and thinking the MTU
was changed leading to unexpected problems. I think it'd be better to
keep the MTU synced explicitly (e.g. when set on main device, then
set it on peer as well) and error out when trying to set it without
the proper capabilities.

Daniel Borkmann May 24, 2024, 2:20 p.m. UTC | #2

On 5/24/24 4:15 PM, Nikolay Aleksandrov wrote:
> On 5/24/24 16:01, Daniel Borkmann wrote:
>> Implement the ndo_change_mtu callback in netkit in order to align the MTU
>> to the primary device. This is needed in order to sync MTUs to the latter
>> from the control plane (e.g. Cilium) which does not have access into the
>> Pod's netns.
>>
>> Fixes: 35dfaad7188c ("netkit, bpf: Add bpf programmable net device")
>> Signed-off-by: Daniel Borkmann <[e-mail]>
>> Cc: Joe Stringer <[e-mail]>
>> ---
>>   drivers/net/netkit.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
> 
> This one has unexpected behaviour IMO. If the app sets the MTU and we
> silently overwrite, then it may continue working and thinking the MTU
> was changed leading to unexpected problems. I think it'd be better to
> keep the MTU synced explicitly (e.g. when set on main device, then
> set it on peer as well) and error out when trying to set it without
> the proper capabilities.

Makes sense, I'll look into this, thanks Nik!

Daniel Borkmann May 24, 2024, 2:30 p.m. UTC | #3

On 5/24/24 4:20 PM, Daniel Borkmann wrote:
> On 5/24/24 4:15 PM, Nikolay Aleksandrov wrote:
>> On 5/24/24 16:01, Daniel Borkmann wrote:
>>> Implement the ndo_change_mtu callback in netkit in order to align the MTU
>>> to the primary device. This is needed in order to sync MTUs to the latter
>>> from the control plane (e.g. Cilium) which does not have access into the
>>> Pod's netns.
>>>
>>> Fixes: 35dfaad7188c ("netkit, bpf: Add bpf programmable net device")
>>> Signed-off-by: Daniel Borkmann <[e-mail]>
>>> Cc: Joe Stringer <[e-mail]>
>>> ---
>>>   drivers/net/netkit.c | 20 ++++++++++++++++++++
>>>   1 file changed, 20 insertions(+)
>>>
>>
>> This one has unexpected behaviour IMO. If the app sets the MTU and we
>> silently overwrite, then it may continue working and thinking the MTU
>> was changed leading to unexpected problems. I think it'd be better to
>> keep the MTU synced explicitly (e.g. when set on main device, then
>> set it on peer as well) and error out when trying to set it without
>> the proper capabilities.
> 
> Makes sense, I'll look into this, thanks Nik!

I'll drop this one for now, and have a future extension on nk device
creation to lock such attributes or not so its flexible.

Thanks,
Daniel

Patch

diff --git a/drivers/net/netkit.c b/drivers/net/netkit.c
index 16789cd446e9..ead7097c224b 100644
--- a/drivers/net/netkit.c
+++ b/drivers/net/netkit.c
@@ -167,6 +167,25 @@  static int netkit_set_macaddr(struct net_device *dev, void *sa)
 	return eth_mac_addr(dev, sa);
 }
 
+static int netkit_set_mtu(struct net_device *dev, int new_mtu)
+{
+	struct netkit *nk = netkit_priv(dev);
+	struct net_device *peer;
+
+	rcu_read_lock();
+	peer = rcu_dereference(nk->peer);
+	if (unlikely(!peer))
+		goto out;
+	if (!nk->primary)
+		new_mtu = READ_ONCE(peer->mtu);
+	else
+		WRITE_ONCE(peer->mtu, new_mtu);
+out:
+	WRITE_ONCE(dev->mtu, new_mtu);
+	rcu_read_unlock();
+	return 0;
+}
+
 static void netkit_set_headroom(struct net_device *dev, int headroom)
 {
 	struct netkit *nk = netkit_priv(dev), *nk2;
@@ -211,6 +230,7 @@  static const struct net_device_ops netkit_netdev_ops = {
 	.ndo_set_rx_mode	= netkit_set_multicast,
 	.ndo_set_rx_headroom	= netkit_set_headroom,
 	.ndo_set_mac_address	= netkit_set_macaddr,
+	.ndo_change_mtu		= netkit_set_mtu,
 	.ndo_get_iflink		= netkit_get_iflink,
 	.ndo_get_peer_dev	= netkit_peer_dev,
 	.ndo_get_stats64	= netkit_get_stats,