* [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
@ 2013-01-10 11:31 Michael S. Tsirkin
2013-01-10 11:55 ` Stefan Hajnoczi
2013-01-10 14:08 ` Jason Wang
0 siblings, 2 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2013-01-10 11:31 UTC (permalink / raw)
To: Jason Wang; +Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel
At the moment, we check owner when we enable queue in tun.
This seems redundant and will break some valid uses
where fd is passed around: I think TUNSETOWNER is there
to prevent others from attaching to a persistent device not
owned by them. Here the fd is already attached,
enabling/disabling queue is more like read/write.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Note: this is unrelated to Stefan's bugfix.
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index fbd106e..78e3225 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
tun = tfile->detached;
if (!tun)
ret = -EINVAL;
- else if (tun_not_capable(tun))
- ret = -EPERM;
else
ret = tun_attach(tun, file);
} else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
tun = rcu_dereference_protected(tfile->tun,
lockdep_rtnl_is_held());
--
MST
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
2013-01-10 11:31 [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE Michael S. Tsirkin
@ 2013-01-10 11:55 ` Stefan Hajnoczi
2013-01-10 22:38 ` David Miller
2013-01-10 14:08 ` Jason Wang
1 sibling, 1 reply; 10+ messages in thread
From: Stefan Hajnoczi @ 2013-01-10 11:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, David S. Miller, netdev, linux-kernel
On Thu, Jan 10, 2013 at 01:31:08PM +0200, Michael S. Tsirkin wrote:
> At the moment, we check owner when we enable queue in tun.
> This seems redundant and will break some valid uses
> where fd is passed around: I think TUNSETOWNER is there
> to prevent others from attaching to a persistent device not
> owned by them. Here the fd is already attached,
> enabling/disabling queue is more like read/write.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> Note: this is unrelated to Stefan's bugfix.
This should go into 3.8-rc.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
2013-01-10 11:31 [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE Michael S. Tsirkin
2013-01-10 11:55 ` Stefan Hajnoczi
@ 2013-01-10 14:08 ` Jason Wang
2013-01-10 14:19 ` Michael S. Tsirkin
1 sibling, 1 reply; 10+ messages in thread
From: Jason Wang @ 2013-01-10 14:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
> At the moment, we check owner when we enable queue in tun.
> This seems redundant and will break some valid uses
> where fd is passed around: I think TUNSETOWNER is there
> to prevent others from attaching to a persistent device not
> owned by them. Here the fd is already attached,
> enabling/disabling queue is more like read/write.
It also change the number of queues of the tuntap, maybe we should limit
this. Note that if management layer does not call TUNSETOWNER, the check
is just a nop. So if management layer want to limit the behavior, it's
its duty to do this correctly.
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> Note: this is unrelated to Stefan's bugfix.
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index fbd106e..78e3225 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
> tun = tfile->detached;
> if (!tun)
> ret = -EINVAL;
> - else if (tun_not_capable(tun))
> - ret = -EPERM;
> else
> ret = tun_attach(tun, file);
> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
> tun = rcu_dereference_protected(tfile->tun,
> lockdep_rtnl_is_held());
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
2013-01-10 14:08 ` Jason Wang
@ 2013-01-10 14:19 ` Michael S. Tsirkin
2013-01-10 14:27 ` Jason Wang
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2013-01-10 14:19 UTC (permalink / raw)
To: Jason Wang
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote:
> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
> > At the moment, we check owner when we enable queue in tun.
> > This seems redundant and will break some valid uses
> > where fd is passed around: I think TUNSETOWNER is there
> > to prevent others from attaching to a persistent device not
> > owned by them. Here the fd is already attached,
> > enabling/disabling queue is more like read/write.
>
> It also change the number of queues of the tuntap, maybe we should limit
> this.
Number of active queues? Why does it matter?
Max number of queues is already limited by SETIFF.
> Note that if management layer does not call TUNSETOWNER, the check
> is just a nop. So if management layer want to limit the behavior, it's
> its duty to do this correctly.
The point is that management limits tun to allow SETIFF from libvirt
only, then passes the fds to qemu.
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ---
> >
> > Note: this is unrelated to Stefan's bugfix.
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index fbd106e..78e3225 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
> > tun = tfile->detached;
> > if (!tun)
> > ret = -EINVAL;
> > - else if (tun_not_capable(tun))
> > - ret = -EPERM;
> > else
> > ret = tun_attach(tun, file);
> > } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
> > tun = rcu_dereference_protected(tfile->tun,
> > lockdep_rtnl_is_held());
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
2013-01-10 14:19 ` Michael S. Tsirkin
@ 2013-01-10 14:27 ` Jason Wang
2013-01-10 14:41 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Jason Wang @ 2013-01-10 14:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote:
>> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
>>> At the moment, we check owner when we enable queue in tun.
>>> This seems redundant and will break some valid uses
>>> where fd is passed around: I think TUNSETOWNER is there
>>> to prevent others from attaching to a persistent device not
>>> owned by them. Here the fd is already attached,
>>> enabling/disabling queue is more like read/write.
>> It also change the number of queues of the tuntap, maybe we should limit
>> this.
> Number of active queues? Why does it matter?
> Max number of queues is already limited by SETIFF.
Yes the number of active(real) queues in the kernel net device and this
changing may introduce other events such uevent. With this patch, even
if a owner is set for tap, every user could change the number of real
queues which I don't think is not expected. Without this patch, we can
limit a user that just do read and write.
>
>> Note that if management layer does not call TUNSETOWNER, the check
>> is just a nop. So if management layer want to limit the behavior, it's
>> its duty to do this correctly.
> The point is that management limits tun to allow SETIFF from libvirt
> only, then passes the fds to qemu.
Yes, but looks like libvirt does not call TUNSETOWNER before passing it
to qemu, so we're ok even without this patch. And if libvirt want to do
this, it can just call TUNSETOWNER to the user of qemu.
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> ---
>>>
>>> Note: this is unrelated to Stefan's bugfix.
>>>
>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>> index fbd106e..78e3225 100644
>>> --- a/drivers/net/tun.c
>>> +++ b/drivers/net/tun.c
>>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
>>> tun = tfile->detached;
>>> if (!tun)
>>> ret = -EINVAL;
>>> - else if (tun_not_capable(tun))
>>> - ret = -EPERM;
>>> else
>>> ret = tun_attach(tun, file);
>>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
>>> tun = rcu_dereference_protected(tfile->tun,
>>> lockdep_rtnl_is_held());
> --
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
2013-01-10 14:27 ` Jason Wang
@ 2013-01-10 14:41 ` Michael S. Tsirkin
2013-01-10 14:47 ` Jason Wang
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2013-01-10 14:41 UTC (permalink / raw)
To: Jason Wang
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
On Thu, Jan 10, 2013 at 10:27:20PM +0800, Jason Wang wrote:
> On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote:
> > On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote:
> >> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
> >>> At the moment, we check owner when we enable queue in tun.
> >>> This seems redundant and will break some valid uses
> >>> where fd is passed around: I think TUNSETOWNER is there
> >>> to prevent others from attaching to a persistent device not
> >>> owned by them. Here the fd is already attached,
> >>> enabling/disabling queue is more like read/write.
> >> It also change the number of queues of the tuntap, maybe we should limit
> >> this.
> > Number of active queues? Why does it matter?
> > Max number of queues is already limited by SETIFF.
>
> Yes the number of active(real) queues in the kernel net device and this
> changing may introduce other events such uevent.
How can it trigger a uevent?
> With this patch, even
> if a owner is set for tap, every user could change the number of real
> queues which I don't think is not expected. Without this patch, we can
> limit a user that just do read and write.
In the end if you want very fine tuned security policy you have to
use an LSM.
Here we are talking about the expected usage without an LSM.
There, enabling/disabling queues is just an optimization:
if an application wants to process data from a single thread
it's better off getting it through a single fd.
Having to channel threading changes through a priveledged
proxy would be very awkward.
> >
> >> Note that if management layer does not call TUNSETOWNER, the check
> >> is just a nop. So if management layer want to limit the behavior, it's
> >> its duty to do this correctly.
> > The point is that management limits tun to allow SETIFF from libvirt
> > only, then passes the fds to qemu.
>
> Yes, but looks like libvirt does not call TUNSETOWNER before passing it
> to qemu, so we're ok even without this patch. And if libvirt want to do
> this, it can just call TUNSETOWNER to the user of qemu.
No, that would allow qemu to do SETIFF which we don't want.
> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>
> >>> ---
> >>>
> >>> Note: this is unrelated to Stefan's bugfix.
> >>>
> >>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> >>> index fbd106e..78e3225 100644
> >>> --- a/drivers/net/tun.c
> >>> +++ b/drivers/net/tun.c
> >>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
> >>> tun = tfile->detached;
> >>> if (!tun)
> >>> ret = -EINVAL;
> >>> - else if (tun_not_capable(tun))
> >>> - ret = -EPERM;
> >>> else
> >>> ret = tun_attach(tun, file);
> >>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
> >>> tun = rcu_dereference_protected(tfile->tun,
> >>> lockdep_rtnl_is_held());
> > --
> > 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
2013-01-10 14:41 ` Michael S. Tsirkin
@ 2013-01-10 14:47 ` Jason Wang
2013-01-10 15:10 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Jason Wang @ 2013-01-10 14:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
On 01/10/2013 10:41 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 10, 2013 at 10:27:20PM +0800, Jason Wang wrote:
>> On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote:
>>> On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote:
>>>> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
>>>>> At the moment, we check owner when we enable queue in tun.
>>>>> This seems redundant and will break some valid uses
>>>>> where fd is passed around: I think TUNSETOWNER is there
>>>>> to prevent others from attaching to a persistent device not
>>>>> owned by them. Here the fd is already attached,
>>>>> enabling/disabling queue is more like read/write.
>>>> It also change the number of queues of the tuntap, maybe we should limit
>>>> this.
>>> Number of active queues? Why does it matter?
>>> Max number of queues is already limited by SETIFF.
>> Yes the number of active(real) queues in the kernel net device and this
>> changing may introduce other events such uevent.
> How can it trigger a uevent?
netif_set_real_num_{tx|rx}_queues() will update the queue kobjects which
may trigger an uevent.
>
>> With this patch, even
>> if a owner is set for tap, every user could change the number of real
>> queues which I don't think is not expected. Without this patch, we can
>> limit a user that just do read and write.
> In the end if you want very fine tuned security policy you have to
> use an LSM.
>
> Here we are talking about the expected usage without an LSM.
> There, enabling/disabling queues is just an optimization:
> if an application wants to process data from a single thread
> it's better off getting it through a single fd.
> Having to channel threading changes through a priveledged
> proxy would be very awkward.
Yes, but we have something similar like bridge-helper in qemu which
create devices through a privileged proxy.
>>>> Note that if management layer does not call TUNSETOWNER, the check
>>>> is just a nop. So if management layer want to limit the behavior, it's
>>>> its duty to do this correctly.
>>> The point is that management limits tun to allow SETIFF from libvirt
>>> only, then passes the fds to qemu.
>> Yes, but looks like libvirt does not call TUNSETOWNER before passing it
>> to qemu, so we're ok even without this patch. And if libvirt want to do
>> this, it can just call TUNSETOWNER to the user of qemu.
> No, that would allow qemu to do SETIFF which we don't want.
True, I was wrong.
>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Note: this is unrelated to Stefan's bugfix.
>>>>>
>>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>>>> index fbd106e..78e3225 100644
>>>>> --- a/drivers/net/tun.c
>>>>> +++ b/drivers/net/tun.c
>>>>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
>>>>> tun = tfile->detached;
>>>>> if (!tun)
>>>>> ret = -EINVAL;
>>>>> - else if (tun_not_capable(tun))
>>>>> - ret = -EPERM;
>>>>> else
>>>>> ret = tun_attach(tun, file);
>>>>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
>>>>> tun = rcu_dereference_protected(tfile->tun,
>>>>> lockdep_rtnl_is_held());
>>> --
>>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
2013-01-10 14:47 ` Jason Wang
@ 2013-01-10 15:10 ` Michael S. Tsirkin
2013-01-10 15:27 ` Jason Wang
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2013-01-10 15:10 UTC (permalink / raw)
To: Jason Wang
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
On Thu, Jan 10, 2013 at 10:47:49PM +0800, Jason Wang wrote:
> On 01/10/2013 10:41 PM, Michael S. Tsirkin wrote:
> > On Thu, Jan 10, 2013 at 10:27:20PM +0800, Jason Wang wrote:
> >> On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote:
> >>> On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote:
> >>>> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
> >>>>> At the moment, we check owner when we enable queue in tun.
> >>>>> This seems redundant and will break some valid uses
> >>>>> where fd is passed around: I think TUNSETOWNER is there
> >>>>> to prevent others from attaching to a persistent device not
> >>>>> owned by them. Here the fd is already attached,
> >>>>> enabling/disabling queue is more like read/write.
> >>>> It also change the number of queues of the tuntap, maybe we should limit
> >>>> this.
> >>> Number of active queues? Why does it matter?
> >>> Max number of queues is already limited by SETIFF.
> >> Yes the number of active(real) queues in the kernel net device and this
> >> changing may introduce other events such uevent.
> > How can it trigger a uevent?
>
> netif_set_real_num_{tx|rx}_queues() will update the queue kobjects which
> may trigger an uevent.
Look SETOWNER is a tool intended mostly for persistent taps,
where you give a specific user the rights to attach to
specific taps but not others.
The uevent issue is preventing a DOS by a uevent flood?
Then it applies to persistent and non persistent as one.
So if one cares about this one should use an LSM
or we can add a separate capability to limit this if we
care enough.
> >
> >> With this patch, even
> >> if a owner is set for tap, every user could change the number of real
> >> queues which I don't think is not expected. Without this patch, we can
> >> limit a user that just do read and write.
> > In the end if you want very fine tuned security policy you have to
> > use an LSM.
> >
> > Here we are talking about the expected usage without an LSM.
> > There, enabling/disabling queues is just an optimization:
> > if an application wants to process data from a single thread
> > it's better off getting it through a single fd.
> > Having to channel threading changes through a priveledged
> > proxy would be very awkward.
>
> Yes, but we have something similar like bridge-helper in qemu which
> create devices through a privileged proxy.
This only happens on startup. Threading changes can happen
at any time.
> >>>> Note that if management layer does not call TUNSETOWNER, the check
> >>>> is just a nop. So if management layer want to limit the behavior, it's
> >>>> its duty to do this correctly.
> >>> The point is that management limits tun to allow SETIFF from libvirt
> >>> only, then passes the fds to qemu.
> >> Yes, but looks like libvirt does not call TUNSETOWNER before passing it
> >> to qemu, so we're ok even without this patch. And if libvirt want to do
> >> this, it can just call TUNSETOWNER to the user of qemu.
> > No, that would allow qemu to do SETIFF which we don't want.
>
> True, I was wrong.
> >
> >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>>
> >>>>> ---
> >>>>>
> >>>>> Note: this is unrelated to Stefan's bugfix.
> >>>>>
> >>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> >>>>> index fbd106e..78e3225 100644
> >>>>> --- a/drivers/net/tun.c
> >>>>> +++ b/drivers/net/tun.c
> >>>>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
> >>>>> tun = tfile->detached;
> >>>>> if (!tun)
> >>>>> ret = -EINVAL;
> >>>>> - else if (tun_not_capable(tun))
> >>>>> - ret = -EPERM;
> >>>>> else
> >>>>> ret = tun_attach(tun, file);
> >>>>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
> >>>>> tun = rcu_dereference_protected(tfile->tun,
> >>>>> lockdep_rtnl_is_held());
> >>> --
> >>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
2013-01-10 15:10 ` Michael S. Tsirkin
@ 2013-01-10 15:27 ` Jason Wang
0 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2013-01-10 15:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
On 01/10/2013 11:10 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 10, 2013 at 10:47:49PM +0800, Jason Wang wrote:
>> On 01/10/2013 10:41 PM, Michael S. Tsirkin wrote:
>>> On Thu, Jan 10, 2013 at 10:27:20PM +0800, Jason Wang wrote:
>>>> On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote:
>>>>> On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote:
>>>>>> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
>>>>>>> At the moment, we check owner when we enable queue in tun.
>>>>>>> This seems redundant and will break some valid uses
>>>>>>> where fd is passed around: I think TUNSETOWNER is there
>>>>>>> to prevent others from attaching to a persistent device not
>>>>>>> owned by them. Here the fd is already attached,
>>>>>>> enabling/disabling queue is more like read/write.
>>>>>> It also change the number of queues of the tuntap, maybe we should limit
>>>>>> this.
>>>>> Number of active queues? Why does it matter?
>>>>> Max number of queues is already limited by SETIFF.
>>>> Yes the number of active(real) queues in the kernel net device and this
>>>> changing may introduce other events such uevent.
>>> How can it trigger a uevent?
>> netif_set_real_num_{tx|rx}_queues() will update the queue kobjects which
>> may trigger an uevent.
> Look SETOWNER is a tool intended mostly for persistent taps,
> where you give a specific user the rights to attach to
> specific taps but not others.
True.
> The uevent issue is preventing a DOS by a uevent flood?
> Then it applies to persistent and non persistent as one.
> So if one cares about this one should use an LSM
> or we can add a separate capability to limit this if we
> care enough.
Ok.
>>>> With this patch, even
>>>> if a owner is set for tap, every user could change the number of real
>>>> queues which I don't think is not expected. Without this patch, we can
>>>> limit a user that just do read and write.
>>> In the end if you want very fine tuned security policy you have to
>>> use an LSM.
>>>
>>> Here we are talking about the expected usage without an LSM.
>>> There, enabling/disabling queues is just an optimization:
>>> if an application wants to process data from a single thread
>>> it's better off getting it through a single fd.
>>> Having to channel threading changes through a priveledged
>>> proxy would be very awkward.
>> Yes, but we have something similar like bridge-helper in qemu which
>> create devices through a privileged proxy.
> This only happens on startup. Threading changes can happen
> at any time.
Yes. So no objection from my side. Thanks for the explanation.
>>>>>> Note that if management layer does not call TUNSETOWNER, the check
>>>>>> is just a nop. So if management layer want to limit the behavior, it's
>>>>>> its duty to do this correctly.
>>>>> The point is that management limits tun to allow SETIFF from libvirt
>>>>> only, then passes the fds to qemu.
>>>> Yes, but looks like libvirt does not call TUNSETOWNER before passing it
>>>> to qemu, so we're ok even without this patch. And if libvirt want to do
>>>> this, it can just call TUNSETOWNER to the user of qemu.
>>> No, that would allow qemu to do SETIFF which we don't want.
>> True, I was wrong.
>>>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> Note: this is unrelated to Stefan's bugfix.
>>>>>>>
>>>>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>>>>>> index fbd106e..78e3225 100644
>>>>>>> --- a/drivers/net/tun.c
>>>>>>> +++ b/drivers/net/tun.c
>>>>>>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
>>>>>>> tun = tfile->detached;
>>>>>>> if (!tun)
>>>>>>> ret = -EINVAL;
>>>>>>> - else if (tun_not_capable(tun))
>>>>>>> - ret = -EPERM;
>>>>>>> else
>>>>>>> ret = tun_attach(tun, file);
>>>>>>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
>>>>>>> tun = rcu_dereference_protected(tfile->tun,
>>>>>>> lockdep_rtnl_is_held());
>>>>> --
>>>>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
2013-01-10 11:55 ` Stefan Hajnoczi
@ 2013-01-10 22:38 ` David Miller
0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2013-01-10 22:38 UTC (permalink / raw)
To: stefanha; +Cc: mst, jasowang, netdev, linux-kernel
From: Stefan Hajnoczi <stefanha@redhat.com>
Date: Thu, 10 Jan 2013 12:55:03 +0100
> On Thu, Jan 10, 2013 at 01:31:08PM +0200, Michael S. Tsirkin wrote:
>> At the moment, we check owner when we enable queue in tun.
>> This seems redundant and will break some valid uses
>> where fd is passed around: I think TUNSETOWNER is there
>> to prevent others from attaching to a persistent device not
>> owned by them. Here the fd is already attached,
>> enabling/disabling queue is more like read/write.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> ---
>>
>> Note: this is unrelated to Stefan's bugfix.
>
> This should go into 3.8-rc.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Applied.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-01-10 22:38 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-10 11:31 [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE Michael S. Tsirkin
2013-01-10 11:55 ` Stefan Hajnoczi
2013-01-10 22:38 ` David Miller
2013-01-10 14:08 ` Jason Wang
2013-01-10 14:19 ` Michael S. Tsirkin
2013-01-10 14:27 ` Jason Wang
2013-01-10 14:41 ` Michael S. Tsirkin
2013-01-10 14:47 ` Jason Wang
2013-01-10 15:10 ` Michael S. Tsirkin
2013-01-10 15:27 ` Jason Wang
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).