lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 17 May 2011 17:54:28 +0300 From: "Michael S. Tsirkin" <mst@...hat.com> To: Michał Mirosław <mirq-linux@...e.qmqm.pl> Cc: netdev@...r.kernel.org, Herbert Xu <herbert@...dor.hengli.com.au>, Ben Hutchings <bhutchings@...arflare.com>, Shan Wei <shanwei@...fujitsu.com> Subject: Re: [PATCH] net: tuntap: Fix tun_net_fix_features() On Tue, May 17, 2011 at 04:46:35PM +0200, Michał Mirosław wrote: > On Tue, May 17, 2011 at 05:29:43PM +0300, Michael S. Tsirkin wrote: > > On Tue, May 17, 2011 at 10:19:54AM +0200, Michał Mirosław wrote: > > > tun->set_features are meant to limit not force the features. > > > > > > Signed-off-by: Michał Mirosław <mirq-linux@...e.qmqm.pl> > > > --- > > > drivers/net/tun.c | 2 +- > > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > > index 74e9405..f77c6d0 100644 > > > --- a/drivers/net/tun.c > > > +++ b/drivers/net/tun.c > > > @@ -458,7 +458,7 @@ 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); > > > + return features & (tun->set_features | ~TUN_USER_FEATURES); > > > } > > > > > > static const struct net_device_ops tun_netdev_ops = { > > > -- > > > 1.7.2.5 > > > > One thing that this will do though: previously, if > > ethtool disables offloads, then an application enables > > them, the application will have the last say. > > With this patch, the most conservative approach wins. > > Right? > > Exactly. > > On device creation, wanted_features default to all offloads > enabled, so unless an admin changes the flags, the application controls > what is enabled. This matters only when using persistent tun/tap and > admin and user are two different people. If the admin is using queues > and doesn't want to handle e.g. TSO packets (I'm not sure if they are > properly accounted in all queuing disciplines), then the feature should > not be enabled by user. > > > If we want to have the existing behaviour > > I think the following would do this (untested). What do you think? > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > index 74e9405..1d6c7bc 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -1199,6 +1199,8 @@ static int set_offload(struct tun_struct *tun, unsigned long arg) > > return -EINVAL; > > > > tun->set_features = features; > > + tun->dev->features &= TUN_USER_FEATURES; > > + tun->dev->features |= (features & TUN_USER_FEATURES); > > netdev_update_features(tun->dev); > > tun->dev->features will be recalculated by netdev_update_features() > anyway. For this to work as you described it would need to alter > wanted_features. I don't like the idea that something other than one > of ethtool_ops is changing this field, as it then becomes something > else that what the admin wants (even if that is not what he gets). > > Best Regards, > Michał Mirosław Yes, with virtualization admin and the app are two different people usually. The device doesn't have to be persistent though I think - what limits this to persistent devices? I agree this behaviour seems more consistent, I just hope this change does not break any setups. -- MST -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists