| Message ID | 20110419161310.7508513909@rere.qmqm.pl |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 19 Apr 2011 18:13:10 +0200 (CEST), Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote: > This changes offload setting behaviour to what I think is correct: > - offloads set via ethtool mean what admin wants to use (by default > he wants 'em all) > - offloads set via ioctl() mean what userspace is expecting to get > (this limits which admin wishes are granted) > - TUN_NOCHECKSUM is ignored, as it might cause broken packets when > forwarded (ip_summed == CHECKSUM_UNNECESSARY means that checksum > was verified, not that it can be ignored) > > If TUN_NOCHECKSUM is implemented, it should set skb->csum_* and > skb->ip_summed (= CHECKSUM_PARTIAL) for known protocols and let others > be verified by kernel when necessary. > > TUN_NOCHECKSUM handling was introduced by commit > f43798c27684ab925adde7d8acc34c78c6e50df8: > > tun: Allow GSO using virtio_net_hdr Err, not in my git tree! It predates git in fact. Since tap requires privs, I wouldn't worry about invalid packets too much. Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: Tue, 19 Apr 2011 18:13:10 +0200 (CEST) > This changes offload setting behaviour to what I think is correct: > - offloads set via ethtool mean what admin wants to use (by default > he wants 'em all) > - offloads set via ioctl() mean what userspace is expecting to get > (this limits which admin wishes are granted) > - TUN_NOCHECKSUM is ignored, as it might cause broken packets when > forwarded (ip_summed == CHECKSUM_UNNECESSARY means that checksum > was verified, not that it can be ignored) > > If TUN_NOCHECKSUM is implemented, it should set skb->csum_* and > skb->ip_summed (= CHECKSUM_PARTIAL) for known protocols and let others > be verified by kernel when necessary. > > TUN_NOCHECKSUM handling was introduced by commit > f43798c27684ab925adde7d8acc34c78c6e50df8: > > tun: Allow GSO using virtio_net_hdr > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 20, 2011 at 12:52:24PM +0930, Rusty Russell wrote: > On Tue, 19 Apr 2011 18:13:10 +0200 (CEST), Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote: > > This changes offload setting behaviour to what I think is correct: > > - offloads set via ethtool mean what admin wants to use (by default > > he wants 'em all) > > - offloads set via ioctl() mean what userspace is expecting to get > > (this limits which admin wishes are granted) > > - TUN_NOCHECKSUM is ignored, as it might cause broken packets when > > forwarded (ip_summed == CHECKSUM_UNNECESSARY means that checksum > > was verified, not that it can be ignored) > > > > If TUN_NOCHECKSUM is implemented, it should set skb->csum_* and > > skb->ip_summed (= CHECKSUM_PARTIAL) for known protocols and let others > > be verified by kernel when necessary. > > > > TUN_NOCHECKSUM handling was introduced by commit > > f43798c27684ab925adde7d8acc34c78c6e50df8: > > > > tun: Allow GSO using virtio_net_hdr > Err, not in my git tree! It predates git in fact. Ah! Sorry! Your patch just moved the buggy line around. > Since tap requires privs, I wouldn't worry about invalid packets too > much. I prefer correctness over uncertainty. ;) Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 20 Apr 2011 01:32:16 -0700 (PDT), David Miller <davem@davemloft.net> wrote: > From: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Date: Tue, 19 Apr 2011 18:13:10 +0200 (CEST) > > > This changes offload setting behaviour to what I think is correct: > > - offloads set via ethtool mean what admin wants to use (by default > > he wants 'em all) > > - offloads set via ioctl() mean what userspace is expecting to get > > (this limits which admin wishes are granted) > > - TUN_NOCHECKSUM is ignored, as it might cause broken packets when > > forwarded (ip_summed == CHECKSUM_UNNECESSARY means that checksum > > was verified, not that it can be ignored) > > > > If TUN_NOCHECKSUM is implemented, it should set skb->csum_* and > > skb->ip_summed (= CHECKSUM_PARTIAL) for known protocols and let others > > be verified by kernel when necessary. > > > > TUN_NOCHECKSUM handling was introduced by commit > > f43798c27684ab925adde7d8acc34c78c6e50df8: > > > > tun: Allow GSO using virtio_net_hdr > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > Applied. Dave, you just removed a feature that has been in Linux since before git. It *probably* just means we go slower in cases we don't really care about. But does removing it break qemu? Has anyone tested? Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index f5e9ac0..ade3cf9 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -123,6 +123,9 @@ struct tun_struct { gid_t group; struct net_device *dev; + u32 set_features; +#define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \ + NETIF_F_TSO6|NETIF_F_UFO) struct fasync_struct *fasync; struct tap_filter txflt; @@ -451,12 +454,20 @@ tun_net_change_mtu(struct net_device *dev, int new_mtu) return 0; } +static u32 tun_net_fix_features(struct net_device *dev, u32 features) +{ + struct tun_struct *tun = netdev_priv(dev); + + return (features & tun->set_features) | (features & ~TUN_USER_FEATURES); +} + static const struct net_device_ops tun_netdev_ops = { .ndo_uninit = tun_net_uninit, .ndo_open = tun_net_open, .ndo_stop = tun_net_close, .ndo_start_xmit = tun_net_xmit, .ndo_change_mtu = tun_net_change_mtu, + .ndo_fix_features = tun_net_fix_features, }; static const struct net_device_ops tap_netdev_ops = { @@ -465,6 +476,7 @@ static const struct net_device_ops tap_netdev_ops = { .ndo_stop = tun_net_close, .ndo_start_xmit = tun_net_xmit, .ndo_change_mtu = tun_net_change_mtu, + .ndo_fix_features = tun_net_fix_features, .ndo_set_multicast_list = tun_net_mclist, .ndo_set_mac_address = eth_mac_addr, .ndo_validate_addr = eth_validate_addr, @@ -628,8 +640,7 @@ static __inline__ ssize_t tun_get_user(struct tun_struct *tun, kfree_skb(skb); return -EINVAL; } - } else if (tun->flags & TUN_NOCHECKSUM) - skb->ip_summed = CHECKSUM_UNNECESSARY; + } switch (tun->flags & TUN_TYPE_MASK) { case TUN_TUN_DEV: @@ -1094,6 +1105,10 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) goto err_free_sk; } + dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | + TUN_USER_FEATURES; + dev->features = dev->hw_features; + err = register_netdevice(tun->dev); if (err < 0) goto err_free_sk; @@ -1158,18 +1173,12 @@ static int tun_get_iff(struct net *net, struct tun_struct *tun, /* This is like a cut-down ethtool ops, except done via tun fd so no * privs required. */ -static int set_offload(struct net_device *dev, unsigned long arg) +static int set_offload(struct tun_struct *tun, unsigned long arg) { - u32 old_features, features; - - old_features = dev->features; - /* Unset features, set them as we chew on the arg. */ - features = (old_features & ~(NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST - |NETIF_F_TSO_ECN|NETIF_F_TSO|NETIF_F_TSO6 - |NETIF_F_UFO)); + u32 features = 0; if (arg & TUN_F_CSUM) { - features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST; + features |= NETIF_F_HW_CSUM; arg &= ~TUN_F_CSUM; if (arg & (TUN_F_TSO4|TUN_F_TSO6)) { @@ -1195,9 +1204,8 @@ static int set_offload(struct net_device *dev, unsigned long arg) if (arg) return -EINVAL; - dev->features = features; - if (old_features != dev->features) - netdev_features_change(dev); + tun->set_features = features; + netdev_update_features(tun->dev); return 0; } @@ -1262,12 +1270,9 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, case TUNSETNOCSUM: /* Disable/Enable checksum */ - if (arg) - tun->flags |= TUN_NOCHECKSUM; - else - tun->flags &= ~TUN_NOCHECKSUM; - tun_debug(KERN_INFO, tun, "checksum %s\n", + /* [unimplemented] */ + tun_debug(KERN_INFO, tun, "ignored: set checksum %s\n", arg ? "disabled" : "enabled"); break; @@ -1316,7 +1321,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, break; #endif case TUNSETOFFLOAD: - ret = set_offload(tun->dev, arg); + ret = set_offload(tun, arg); break; case TUNSETTXFILTER: @@ -1595,30 +1600,12 @@ static void tun_set_msglevel(struct net_device *dev, u32 value) #endif } -static u32 tun_get_rx_csum(struct net_device *dev) -{ - struct tun_struct *tun = netdev_priv(dev); - return (tun->flags & TUN_NOCHECKSUM) == 0; -} - -static int tun_set_rx_csum(struct net_device *dev, u32 data) -{ - struct tun_struct *tun = netdev_priv(dev); - if (data) - tun->flags &= ~TUN_NOCHECKSUM; - else - tun->flags |= TUN_NOCHECKSUM; - return 0; -} - static const struct ethtool_ops tun_ethtool_ops = { .get_settings = tun_get_settings, .get_drvinfo = tun_get_drvinfo, .get_msglevel = tun_get_msglevel, .set_msglevel = tun_set_msglevel, .get_link = ethtool_op_get_link, - .get_rx_csum = tun_get_rx_csum, - .set_rx_csum = tun_set_rx_csum };
This changes offload setting behaviour to what I think is correct: - offloads set via ethtool mean what admin wants to use (by default he wants 'em all) - offloads set via ioctl() mean what userspace is expecting to get (this limits which admin wishes are granted) - TUN_NOCHECKSUM is ignored, as it might cause broken packets when forwarded (ip_summed == CHECKSUM_UNNECESSARY means that checksum was verified, not that it can be ignored) If TUN_NOCHECKSUM is implemented, it should set skb->csum_* and skb->ip_summed (= CHECKSUM_PARTIAL) for known protocols and let others be verified by kernel when necessary. TUN_NOCHECKSUM handling was introduced by commit f43798c27684ab925adde7d8acc34c78c6e50df8: tun: Allow GSO using virtio_net_hdr Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- drivers/net/tun.c | 63 +++++++++++++++++++++-------------------------------- 1 files changed, 25 insertions(+), 38 deletions(-)