diff mbox series

net: rmnet: Adjust virtual device MTU on real device capability

Message ID 1607017240-10582-1-git-send-email-loic.poulain@linaro.org
State New
Headers show
Series net: rmnet: Adjust virtual device MTU on real device capability | expand

Commit Message

Loic Poulain Dec. 3, 2020, 5:40 p.m. UTC
A submitted qmap/rmnet packet size can not be larger than the linked
interface (real_dev) MTU. This patch ensures that the rmnet virtual
iface MTU is configured according real device capability.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.7.4

Comments

Subash Abhinov Kasiviswanathan Dec. 4, 2020, 9:56 p.m. UTC | #1
On 2020-12-03 10:40, Loic Poulain wrote:
> A submitted qmap/rmnet packet size can not be larger than the linked

> interface (real_dev) MTU. This patch ensures that the rmnet virtual

> iface MTU is configured according real device capability.

> 

> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

> ---

>  drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c | 8 ++++++++

>  1 file changed, 8 insertions(+)

> 

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c

> b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c

> index d58b51d..d1d7328 100644

> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c

> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c

> @@ -60,9 +60,14 @@ static netdev_tx_t rmnet_vnd_start_xmit(struct 

> sk_buff

> *skb,

> 

>  static int rmnet_vnd_change_mtu(struct net_device *rmnet_dev, int

> new_mtu)

>  {

> +	struct rmnet_priv *priv = netdev_priv(rmnet_dev);

> +

>  	if (new_mtu < 0 || new_mtu > RMNET_MAX_PACKET_SIZE)

>  		return -EINVAL;

> 

> +	if (priv->real_dev && (new_mtu + RMNET_NEEDED_HEADROOM) >

> priv->real_dev->mtu)

> +		return -EINVAL;

> +

>  	rmnet_dev->mtu = new_mtu;

>  	return 0;

>  }

> @@ -242,6 +247,9 @@ int rmnet_vnd_newlink(u8 id, struct net_device

> *rmnet_dev,

> 

>  	priv->real_dev = real_dev;

> 

> +	/* Align default MTU with real_dev MTU */

> +	rmnet_vnd_change_mtu(rmnet_dev, real_dev->mtu -

> RMNET_NEEDED_HEADROOM);

> +

>  	rc = register_netdevice(rmnet_dev);

>  	if (!rc) {

>  		ep->egress_dev = rmnet_dev;


This would need similar checks in the NETDEV_PRECHANGEMTU
netdev notifier.
Loic Poulain Dec. 7, 2020, 4:45 p.m. UTC | #2
Hi Subash,

On Fri, 4 Dec 2020 at 22:56, <subashab@codeaurora.org> wrote:
>

> On 2020-12-03 10:40, Loic Poulain wrote:

> > A submitted qmap/rmnet packet size can not be larger than the linked

> > interface (real_dev) MTU. This patch ensures that the rmnet virtual

> > iface MTU is configured according real device capability.

> >

> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

[...]
> > @@ -242,6 +247,9 @@ int rmnet_vnd_newlink(u8 id, struct net_device

> > *rmnet_dev,

> >

> >       priv->real_dev = real_dev;

> >

> > +     /* Align default MTU with real_dev MTU */

> > +     rmnet_vnd_change_mtu(rmnet_dev, real_dev->mtu -

> > RMNET_NEEDED_HEADROOM);

> > +

> >       rc = register_netdevice(rmnet_dev);

> >       if (!rc) {

> >               ep->egress_dev = rmnet_dev;

>

> This would need similar checks in the NETDEV_PRECHANGEMTU

> netdev notifier.


What about just returning an error on NETDEV_PRECHANGEMTU notification
to prevent real device MTU change while virtual rmnet devices are
linked? Not sure there is a more proper and thread safe way to manager
that otherwise.

Regards,
Loic
Jakub Kicinski Dec. 7, 2020, 8:16 p.m. UTC | #3
On Mon, 7 Dec 2020 17:45:17 +0100 Loic Poulain wrote:
> > This would need similar checks in the NETDEV_PRECHANGEMTU

> > netdev notifier.  

> 

> What about just returning an error on NETDEV_PRECHANGEMTU notification

> to prevent real device MTU change while virtual rmnet devices are

> linked? Not sure there is a more proper and thread safe way to manager

> that otherwise.


Can't you copy what vlan devices do?  That'd seem like a reasonable and
well tested precedent, no?
Subash Abhinov Kasiviswanathan Dec. 8, 2020, 8:19 a.m. UTC | #4
>> What about just returning an error on NETDEV_PRECHANGEMTU notification

>> to prevent real device MTU change while virtual rmnet devices are

>> linked? Not sure there is a more proper and thread safe way to manager

>> that otherwise.

> 

> Can't you copy what vlan devices do?  That'd seem like a reasonable and

> well tested precedent, no?


Could you try this patch. I've tried addressing most of the conditions 
here.
I haven't seen any issues with updating the MTU when rmnet devices are 
linked.

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c 
b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
index fcdecdd..8d51b0c 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
@@ -26,7 +26,7 @@ static int rmnet_is_real_dev_registered(const struct 
net_device *real_dev)
  }

  /* Needs rtnl lock */
-static struct rmnet_port*
+struct rmnet_port*
  rmnet_get_port_rtnl(const struct net_device *real_dev)
  {
  	return rtnl_dereference(real_dev->rx_handler_data);
@@ -253,7 +253,10 @@ static int rmnet_config_notify_cb(struct 
notifier_block *nb,
  		netdev_dbg(real_dev, "Kernel unregister\n");
  		rmnet_force_unassociate_device(real_dev);
  		break;
-
+	case NETDEV_CHANGEMTU:
+		if (rmnet_vnd_validate_real_dev_mtu(real_dev))
+			return NOTIFY_BAD;
+		break;
  	default:
  		break;
  	}
@@ -329,9 +332,17 @@ static int rmnet_changelink(struct net_device *dev, 
struct nlattr *tb[],

  	if (data[IFLA_RMNET_FLAGS]) {
  		struct ifla_rmnet_flags *flags;
+		u32 old_data_format;

+		old_data_format = port->data_format;
  		flags = nla_data(data[IFLA_RMNET_FLAGS]);
  		port->data_format = flags->flags & flags->mask;
+
+		if (rmnet_vnd_update_dev_mtu(port, real_dev)) {
+			port->data_format = old_data_format;
+			NL_SET_ERR_MSG_MOD(extack, "Invalid MTU on real dev");
+			return -EINVAL;
+		}
  	}

  	return 0;
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h 
b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
index be51598..8d8d469 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
@@ -73,4 +73,6 @@ int rmnet_add_bridge(struct net_device *rmnet_dev,
  		     struct netlink_ext_ack *extack);
  int rmnet_del_bridge(struct net_device *rmnet_dev,
  		     struct net_device *slave_dev);
+struct rmnet_port*
+rmnet_get_port_rtnl(const struct net_device *real_dev);
  #endif /* _RMNET_CONFIG_H_ */
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c 
b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
index d58b51d..df87883 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
@@ -58,9 +58,30 @@ static netdev_tx_t rmnet_vnd_start_xmit(struct 
sk_buff *skb,
  	return NETDEV_TX_OK;
  }

+static int rmnet_vnd_headroom(struct net_device *real_dev)
+{
+	struct rmnet_port *port;
+	u32 headroom;
+
+	port = rmnet_get_port_rtnl(real_dev);
+
+	headroom = sizeof(struct rmnet_map_header);
+
+	if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
+		headroom += sizeof(struct rmnet_map_dl_csum_trailer);
+
+	return headroom;
+}
+
  static int rmnet_vnd_change_mtu(struct net_device *rmnet_dev, int 
new_mtu)
  {
-	if (new_mtu < 0 || new_mtu > RMNET_MAX_PACKET_SIZE)
+	struct rmnet_priv *priv = netdev_priv(rmnet_dev);
+	u32 headroom;
+
+	headroom = rmnet_vnd_headroom(priv->real_dev);
+
+	if (new_mtu < 0 || new_mtu > RMNET_MAX_PACKET_SIZE ||
+	    new_mtu > (priv->real_dev->mtu - headroom))
  		return -EINVAL;

  	rmnet_dev->mtu = new_mtu;
@@ -229,6 +250,7 @@ int rmnet_vnd_newlink(u8 id, struct net_device 
*rmnet_dev,

  {
  	struct rmnet_priv *priv = netdev_priv(rmnet_dev);
+	u32 headroom;
  	int rc;

  	if (rmnet_get_endpoint(port, id)) {
@@ -242,6 +264,13 @@ int rmnet_vnd_newlink(u8 id, struct net_device 
*rmnet_dev,

  	priv->real_dev = real_dev;

+	headroom = rmnet_vnd_headroom(real_dev);
+
+	if (rmnet_vnd_change_mtu(rmnet_dev, real_dev->mtu - headroom)) {
+		NL_SET_ERR_MSG_MOD(extack, "Invalid MTU on real dev");
+		return -EINVAL;
+	}
+
  	rc = register_netdevice(rmnet_dev);
  	if (!rc) {
  		ep->egress_dev = rmnet_dev;
@@ -283,3 +312,51 @@ int rmnet_vnd_do_flow_control(struct net_device 
*rmnet_dev, int enable)

  	return 0;
  }
+
+int rmnet_vnd_validate_real_dev_mtu(struct net_device *real_dev)
+{
+	struct hlist_node *tmp_ep;
+	struct rmnet_endpoint *ep;
+	struct rmnet_port *port;
+	unsigned long bkt_ep;
+	u32 headroom;
+
+	port = rmnet_get_port_rtnl(real_dev);
+
+	headroom = sizeof(struct rmnet_map_header);
+
+	if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
+		headroom += sizeof(struct rmnet_map_dl_csum_trailer);
+
+	hash_for_each_safe(port->muxed_ep, bkt_ep, tmp_ep, ep, hlnode) {
+		if (ep->egress_dev->mtu > (real_dev->mtu - headroom))
+			return -1;
+	}
+
+	return 0;
+}
+
+int rmnet_vnd_update_dev_mtu(struct rmnet_port *port,
+			     struct net_device *real_dev)
+{
+	struct hlist_node *tmp_ep;
+	struct rmnet_endpoint *ep;
+	unsigned long bkt_ep;
+	u32 headroom;
+
+	headroom = sizeof(struct rmnet_map_header);
+
+	if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
+		headroom += sizeof(struct rmnet_map_dl_csum_trailer);
+
+	hash_for_each_safe(port->muxed_ep, bkt_ep, tmp_ep, ep, hlnode) {
+		if (ep->egress_dev->mtu <= (real_dev->mtu - headroom))
+			continue;
+
+		if (rmnet_vnd_change_mtu(ep->egress_dev,
+					 real_dev->mtu - headroom))
+			return -1;
+	}
+
+	return 0;
+}
\ No newline at end of file
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.h 
b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.h
index 4967f34..dc3a444 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.h
@@ -18,4 +18,7 @@ int rmnet_vnd_dellink(u8 id, struct rmnet_port *port,
  void rmnet_vnd_rx_fixup(struct sk_buff *skb, struct net_device *dev);
  void rmnet_vnd_tx_fixup(struct sk_buff *skb, struct net_device *dev);
  void rmnet_vnd_setup(struct net_device *dev);
+int rmnet_vnd_validate_real_dev_mtu(struct net_device *real_dev);
+int rmnet_vnd_update_dev_mtu(struct rmnet_port *port,
+			     struct net_device *real_dev);
  #endif /* _RMNET_VND_H_ */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project
Loic Poulain Dec. 8, 2020, 2:50 p.m. UTC | #5
Hi Subash,

On Tue, 8 Dec 2020 at 09:19, <subashab@codeaurora.org> wrote:
>

> >> What about just returning an error on NETDEV_PRECHANGEMTU notification

> >> to prevent real device MTU change while virtual rmnet devices are

> >> linked? Not sure there is a more proper and thread safe way to manager

> >> that otherwise.

> >

> > Can't you copy what vlan devices do?  That'd seem like a reasonable and

> > well tested precedent, no?

>

> Could you try this patch. I've tried addressing most of the conditions

> here.

> I haven't seen any issues with updating the MTU when rmnet devices are

> linked.


Thanks, could you submit it as new regular patch, I'll give you my tested-by.

Regards,
Loic
diff mbox series

Patch

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
index d58b51d..d1d7328 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
@@ -60,9 +60,14 @@  static netdev_tx_t rmnet_vnd_start_xmit(struct sk_buff *skb,
 
 static int rmnet_vnd_change_mtu(struct net_device *rmnet_dev, int new_mtu)
 {
+	struct rmnet_priv *priv = netdev_priv(rmnet_dev);
+
 	if (new_mtu < 0 || new_mtu > RMNET_MAX_PACKET_SIZE)
 		return -EINVAL;
 
+	if (priv->real_dev && (new_mtu + RMNET_NEEDED_HEADROOM) > priv->real_dev->mtu)
+		return -EINVAL;
+
 	rmnet_dev->mtu = new_mtu;
 	return 0;
 }
@@ -242,6 +247,9 @@  int rmnet_vnd_newlink(u8 id, struct net_device *rmnet_dev,
 
 	priv->real_dev = real_dev;
 
+	/* Align default MTU with real_dev MTU */
+	rmnet_vnd_change_mtu(rmnet_dev, real_dev->mtu - RMNET_NEEDED_HEADROOM);
+
 	rc = register_netdevice(rmnet_dev);
 	if (!rc) {
 		ep->egress_dev = rmnet_dev;