Re: [PATCH,v2,net-next 2/2] tun: enable napi_gro_frags() for TUN/TAP driver

On Fri, Sep 22, 2017 at 9:51 AM, Mahesh Bandewar (महेश बंडेवार)
[off-list ref] wrote:

On Fri, Sep 22, 2017 at 7:06 AM, Willem de Bruijn
[off-list ref] wrote:

quoted

quoted

@@ -2061,6 +2174,9 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
        if (tfile->detached)
                return -EINVAL;

+       if ((ifr->ifr_flags & IFF_NAPI_FRAGS) && !capable(CAP_NET_ADMIN))
+               return -EPERM;
+

This should perhaps be moved into the !dev branch, directly below the
ns_capable check.

Hmm, does that mean fail only on creation but allow to attach if
exists? That would be wrong, isn't it? Correct me if I'm wrong but we
want to prevent both these scenarios if user does not have sufficient
privileges (i.e. NET_ADMIN in init-ns).

My understanding is we want to protect both scenarios.

quoted

quoted

        dev = __dev_get_by_name(net, ifr->ifr_name);
        if (dev) {
                if (ifr->ifr_flags & IFF_TUN_EXCL)

@@ -2185,6 +2301,9 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
        tun->flags = (tun->flags & ~TUN_FEATURES) |
                (ifr->ifr_flags & TUN_FEATURES);

+       if (!(tun->flags & IFF_NAPI) || (tun->flags & TUN_TYPE_MASK) != IFF_TAP)
+               tun->flags = tun->flags & ~IFF_NAPI_FRAGS;
+

Similarly, this check only need to be performed in that branch.
Instead of reverting to non-frags mode, a tun_set_iff with the wrong
set of flags should probably fail hard.

Yes, agree, wrong set of flags should fail hard and probably be done
before attach or open, no?

Agreed, in v3 I will push this check before the conditional so both
branches can be rejected with EINVAL.