From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Herbert Subject: Re: [PATCH net-next 14/16] net: Add sk_bind_dev_if to task_struct Date: Tue, 28 Jul 2015 10:12:26 -0700 Message-ID: References: <1438021869-49186-1-git-send-email-dsa@cumulusnetworks.com> <1438021869-49186-15-git-send-email-dsa@cumulusnetworks.com> <55B7A9A1.6010704@cumulusnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Andy Lutomirski , gospo@cumulusnetworks.com, "Eric W. Biederman" , ddutt@cumulusnetworks.com, Hannes Frederic Sowa , Ingo Molnar , Stephen Hemminger , Nicolas Dichtel , jtoppins@cumulusnetworks.com, Network Development , "David S. Miller" , Shrijeet Mukherjee , svaidya@brocade.com, hadi@mojatatu.com, nikolay@cumulusnetworks.com, Roopa Prabhu , Blake Matheny , Alex Gartrell To: David Ahern Return-path: Received: from mail-ig0-f177.google.com ([209.85.213.177]:36145 "EHLO mail-ig0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752262AbbG1RM0 (ORCPT ); Tue, 28 Jul 2015 13:12:26 -0400 Received: by igbij6 with SMTP id ij6so125223043igb.1 for ; Tue, 28 Jul 2015 10:12:26 -0700 (PDT) In-Reply-To: <55B7A9A1.6010704@cumulusnetworks.com> Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Jul 28, 2015 at 9:11 AM, David Ahern wrote: > On 7/28/15 9:25 AM, Andy Lutomirski wrote: >> >> On Jul 27, 2015 11:33 AM, "David Ahern" wrote: >>> >>> >>> Allow tasks to have a default device index for binding sockets. If set >>> the value is passed to all AF_INET/AF_INET6 sockets when they are >>> created. >>> >> >> This is not intended to be a review of the concept. I haven't thought >> about whether the concept is a good idea, broken by design, or >> whatever. FWIW, if this were added to the kernel and didn't require >> excessive privilege, I'd probably use it. (I still don't really >> understand why binding to a device requires privilege in the first >> place, but, again, I haven't thought about it very much.) > > > The intent here is to restrict a task to only sending and receiving packets > from a single network device. The device can be single ethernet interface, a > stacked device (e.g, bond) or in our case a VRF device which restricts a > task to interfaces (and hence network paths) associated with the VRF. > We are also intending to implement similar functionality for ILA to restrict tasks (probably from cgroup) to binding to it's assigned addresses. This seems most easily accomplished by adding a binding interface which is only checked at bind time. After binding, the a connection should be processed no differently than any others, additional plumbing in the data path for network name spaces just seems like overhead. Tom >> >>> +#ifdef CONFIG_NET >>> + case PR_SET_SK_BIND_DEV_IF: >>> + { >>> + struct net_device *dev; >>> + int idx = (int) arg2; >>> + >>> + if (!capable(CAP_NET_ADMIN)) >>> + return -EPERM; >>> + >> >> >> Can you either use ns_capable or add a comment as to why not? > > > will do. > >> >> Also, please return -EINVAL if unused args are nonzero. > > > ok. > >> >>> + if (idx) { >>> + dev = dev_get_by_index(me->nsproxy->net_ns, idx); >>> + if (!dev) >>> + return -EINVAL; >>> + dev_put(dev); >>> + } >>> + me->sk_bind_dev_if = idx; >>> + break; >>> + } >>> + case PR_GET_SK_BIND_DEV_IF: >>> + { >>> + struct task_struct *tsk; >>> + int sk_bind_dev_if = -EINVAL; >>> + >>> + rcu_read_lock(); >>> + tsk = find_task_by_vpid(arg2); >>> + if (tsk) >>> + sk_bind_dev_if = tsk->sk_bind_dev_if; >> >> >> Why do you support different tasks here? Could this use proc instead? > > > In this case we want to allow a separate process to determine if a task is > restricted to a device. > >> >> The same -EINVAL issue applies. >> >> Also, I think you need to hook setns and unshare to do something >> reasonable when the task is bound to a device. > > > ack on both. > > Thanks for the review, > David > -- > 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