On Tue, Jul 28, 2015 at 9:11 AM, David Ahern <d...@cumulusnetworks.com> wrote:
> On 7/28/15 9:25 AM, Andy Lutomirski wrote:
>>
>> On Jul 27, 2015 11:33 AM, "David Ahern" <d...@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.
>
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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
