From: David Ahern <dsa@cumulusnetworks.com>
To: Andy Lutomirski <luto@amacapital.net>
Cc: gospo@cumulusnetworks.com,
"Eric W. Biederman" <ebiederm@xmission.com>,
ddutt@cumulusnetworks.com,
Hannes Frederic Sowa <hannes@stressinduktion.org>,
Ingo Molnar <mingo@kernel.org>,
Stephen Hemminger <stephen@networkplumber.org>,
Nicolas Dichtel <nicolas.dichtel@6wind.com>,
jtoppins@cumulusnetworks.com,
Network Development <netdev@vger.kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Shrijeet Mukherjee <shm@cumulusnetworks.com>,
svaidya@brocade.com, hadi@mojatatu.com,
nikolay@cumulusnetworks.com, roopa@cumulusnetworks.com
Subject: Re: [PATCH net-next 14/16] net: Add sk_bind_dev_if to task_struct
Date: Tue, 28 Jul 2015 10:11:13 -0600 [thread overview]
Message-ID: <55B7A9A1.6010704@cumulusnetworks.com> (raw)
In-Reply-To: <CALCETrWbvbRK5rE3O-1AO9WSFxvWnbYMyYNu953SmiaEU1Q0XQ@mail.gmail.com>
On 7/28/15 9:25 AM, Andy Lutomirski wrote:
> On Jul 27, 2015 11:33 AM, "David Ahern" <dsa@cumulusnetworks.com> 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.
>
>> +#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
next prev parent reply other threads:[~2015-07-28 16:11 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-27 18:30 [net-next 0/16] Proposal for VRF-lite - v3 David Ahern
2015-07-27 18:30 ` [PATCH net-next 01/16] net: Refactor rtable allocation and initialization David Ahern
2015-07-27 18:30 ` [PATCH net-next 02/16] net: export a few FIB functions David Ahern
2015-07-27 18:30 ` [PATCH net-next 03/16] net: Introduce VRF related flags and helpers David Ahern
2015-07-27 18:30 ` [PATCH net-next 04/16] net: Use VRF device index for lookups on RX David Ahern
2015-07-27 18:30 ` [PATCH net-next 05/16] net: Use VRF device index for lookups on TX David Ahern
2015-07-27 18:30 ` [PATCH net-next 06/16] net: Tx via VRF device David Ahern
2015-07-27 18:31 ` [PATCH net-next 07/16] net: Add inet_addr lookup by table David Ahern
2015-07-27 18:31 ` [PATCH net-next 08/16] net: Fix up inet_addr_type checks David Ahern
2015-07-27 18:31 ` [PATCH net-next 09/16] net: Add routes to the table associated with the device David Ahern
2015-07-27 18:31 ` [PATCH net-next 10/16] net: Use passed in table for nexthop lookups David Ahern
2015-07-27 18:31 ` [PATCH net-next 11/16] net: Use VRF device index for socket lookups David Ahern
2015-07-27 18:31 ` [PATCH net-next 12/16] net: Add ipv4 route helper to set next hop David Ahern
2015-07-27 18:31 ` [PATCH net-next 13/16] net: Introduce VRF device driver - v2 David Ahern
2015-07-27 20:01 ` Nikolay Aleksandrov
2015-07-28 16:22 ` David Ahern
2015-07-27 18:31 ` [PATCH net-next 14/16] net: Add sk_bind_dev_if to task_struct David Ahern
2015-07-27 20:33 ` Eric W. Biederman
2015-07-28 12:19 ` Hannes Frederic Sowa
2015-07-28 13:54 ` Eric W. Biederman
2015-07-28 14:20 ` Hannes Frederic Sowa
2015-07-28 16:01 ` Eric Dumazet
2015-07-28 16:07 ` David Ahern
2015-07-28 16:52 ` Eric Dumazet
2015-07-28 15:25 ` Andy Lutomirski
2015-07-28 16:11 ` David Ahern [this message]
2015-07-28 17:12 ` Tom Herbert
2015-07-27 18:31 ` [PATCH net-next 15/16] net: Add chvrf command David Ahern
2015-07-27 18:31 ` [PATCH] iproute2: Add support for VRF device David Ahern
2015-07-27 20:30 ` [net-next 0/16] Proposal for VRF-lite - v3 Eric W. Biederman
2015-07-28 16:02 ` David Ahern
2015-07-28 17:07 ` Eric W. Biederman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55B7A9A1.6010704@cumulusnetworks.com \
--to=dsa@cumulusnetworks.com \
--cc=davem@davemloft.net \
--cc=ddutt@cumulusnetworks.com \
--cc=ebiederm@xmission.com \
--cc=gospo@cumulusnetworks.com \
--cc=hadi@mojatatu.com \
--cc=hannes@stressinduktion.org \
--cc=jtoppins@cumulusnetworks.com \
--cc=luto@amacapital.net \
--cc=mingo@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nicolas.dichtel@6wind.com \
--cc=nikolay@cumulusnetworks.com \
--cc=roopa@cumulusnetworks.com \
--cc=shm@cumulusnetworks.com \
--cc=stephen@networkplumber.org \
--cc=svaidya@brocade.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).