Sometimes vfio iommu type1 needs to pin memory for a remote task other than current, thereby needs to test the CAP_IPC_LOCK capability for that task. The proper routine for this purpose is has_capability(), but it is not yet exported for modules. None of currently exported capability- testing symbols allows a specified task. So here has_capability() is exported then used in the vfio iommu type1 driver. Jike Song (2): capability: export has_capability vfio iommu type1: fix the testing of capability for remote task drivers/vfio/vfio_iommu_type1.c | 6 ++---- kernel/capability.c | 1 + 2 files changed, 3 insertions(+), 4 deletions(-) -- 2.4.4.488.gdf97e5d
has_capability() is sometimes needed by modules to test capability for specified task other than current, so export it. Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Kirti Wankhede <kwankhede@nvidia.com> Signed-off-by: Jike Song <jike.song@intel.com> --- kernel/capability.c | 1 + 1 file changed, 1 insertion(+) diff --git a/kernel/capability.c b/kernel/capability.c index 4984e1f..e2e198c 100644 --- a/kernel/capability.c +++ b/kernel/capability.c @@ -318,6 +318,7 @@ bool has_capability(struct task_struct *t, int cap) { return has_ns_capability(t, &init_user_ns, cap); } +EXPORT_SYMBOL(has_capability); /** * has_ns_capability_noaudit - Does a task have a capability (unaudited) -- 2.4.4.488.gdf97e5d
Before the mdev enhancement type1 iommu used capable() to test the capability of current task; in the course of mdev development a new requirement, testing for another task other than current, was raised. ns_capable() was used for this purpose, however it still tests current, the only difference is, in a specified namespace. Fix it by using has_capability() instead, which tests the cap for specified task in init_user_ns, the same namespace as capable(). Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Kirti Wankhede <kwankhede@nvidia.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Jike Song <jike.song@intel.com> --- drivers/vfio/vfio_iommu_type1.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index f3726ba..b54aedf 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -394,8 +394,7 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, long npage, unsigned long *pfn_base) { unsigned long limit; - bool lock_cap = ns_capable(task_active_pid_ns(dma->task)->user_ns, - CAP_IPC_LOCK); + bool lock_cap = has_capability(dma->task, CAP_IPC_LOCK); struct mm_struct *mm; long ret, i = 0, lock_acct = 0; bool rsvd; @@ -491,8 +490,7 @@ static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr, unsigned long *pfn_base, bool do_accounting) { unsigned long limit; - bool lock_cap = ns_capable(task_active_pid_ns(dma->task)->user_ns, - CAP_IPC_LOCK); + bool lock_cap = has_capability(dma->task, CAP_IPC_LOCK); struct mm_struct *mm; int ret; bool rsvd; -- 2.4.4.488.gdf97e5d
On 12/21/2016 9:40 PM, Jike Song wrote: > Before the mdev enhancement type1 iommu used capable() to test the > capability of current task; in the course of mdev development a > new requirement, testing for another task other than current, was > raised. ns_capable() was used for this purpose, however it still > tests current, the only difference is, in a specified namespace. > > Fix it by using has_capability() instead, which tests the cap for > specified task in init_user_ns, the same namespace as capable(). > > Cc: Alex Williamson <alex.williamson@redhat.com> > Cc: Kirti Wankhede <kwankhede@nvidia.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Jike Song <jike.song@intel.com> > --- > drivers/vfio/vfio_iommu_type1.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index f3726ba..b54aedf 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -394,8 +394,7 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > long npage, unsigned long *pfn_base) > { > unsigned long limit; > - bool lock_cap = ns_capable(task_active_pid_ns(dma->task)->user_ns, > - CAP_IPC_LOCK); > + bool lock_cap = has_capability(dma->task, CAP_IPC_LOCK); Hi Jike, Alex's patch already changes this to capable(), you need to resolve. https://lkml.org/lkml/2016/12/20/490 You need to do only below change, which looks fine to me. > struct mm_struct *mm; > long ret, i = 0, lock_acct = 0; > bool rsvd; > @@ -491,8 +490,7 @@ static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr, > unsigned long *pfn_base, bool do_accounting) > { > unsigned long limit; > - bool lock_cap = ns_capable(task_active_pid_ns(dma->task)->user_ns, > - CAP_IPC_LOCK); > + bool lock_cap = has_capability(dma->task, CAP_IPC_LOCK); > struct mm_struct *mm; > int ret; > bool rsvd; > Thanks, Kirti
On 12/22/2016 08:20 PM, Kirti Wankhede wrote: > On 12/21/2016 9:40 PM, Jike Song wrote: >> Before the mdev enhancement type1 iommu used capable() to test the >> capability of current task; in the course of mdev development a >> new requirement, testing for another task other than current, was >> raised. ns_capable() was used for this purpose, however it still >> tests current, the only difference is, in a specified namespace. >> >> Fix it by using has_capability() instead, which tests the cap for >> specified task in init_user_ns, the same namespace as capable(). >> >> Cc: Alex Williamson <alex.williamson@redhat.com> >> Cc: Kirti Wankhede <kwankhede@nvidia.com> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Signed-off-by: Jike Song <jike.song@intel.com> >> --- >> drivers/vfio/vfio_iommu_type1.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >> index f3726ba..b54aedf 100644 >> --- a/drivers/vfio/vfio_iommu_type1.c >> +++ b/drivers/vfio/vfio_iommu_type1.c >> @@ -394,8 +394,7 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, >> long npage, unsigned long *pfn_base) >> { >> unsigned long limit; >> - bool lock_cap = ns_capable(task_active_pid_ns(dma->task)->user_ns, >> - CAP_IPC_LOCK); >> + bool lock_cap = has_capability(dma->task, CAP_IPC_LOCK); > > > Hi Jike, > > Alex's patch already changes this to capable(), you need to resolve. > https://lkml.org/lkml/2016/12/20/490 > > You need to do only below change, which looks fine to me. > Thanks for the point, will change it in v2. However, that will probably be after patch 1/2 accepted, otherwise we get undefined symbols. -- Thanks, Jike >> struct mm_struct *mm; >> long ret, i = 0, lock_acct = 0; >> bool rsvd; >> @@ -491,8 +490,7 @@ static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr, >> unsigned long *pfn_base, bool do_accounting) >> { >> unsigned long limit; >> - bool lock_cap = ns_capable(task_active_pid_ns(dma->task)->user_ns, >> - CAP_IPC_LOCK); >> + bool lock_cap = has_capability(dma->task, CAP_IPC_LOCK); >> struct mm_struct *mm; >> int ret; >> bool rsvd; >>
On Thu, 22 Dec 2016 00:10:15 +0800
Jike Song <jike.song@intel.com> wrote:
> has_capability() is sometimes needed by modules to test capability
> for specified task other than current, so export it.
>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> ---
> kernel/capability.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 4984e1f..e2e198c 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -318,6 +318,7 @@ bool has_capability(struct task_struct *t, int cap)
> {
> return has_ns_capability(t, &init_user_ns, cap);
> }
> +EXPORT_SYMBOL(has_capability);
>
> /**
> * has_ns_capability_noaudit - Does a task have a capability (unaudited)
Are we using EXPORT_SYMBOL vs EXPORT_SYMBOL_GPL here to match the other
exports in this file? We could use _GPL to match the expected caller
of this.
Serge,
Do you have any comments on this patch? I'd be happy to pull it
through the vfio tree with an appropriate Ack. Thanks,
Alex
On 01/12/2017 02:47 AM, Alex Williamson wrote: > On Thu, 22 Dec 2016 00:10:15 +0800 > Jike Song <jike.song@intel.com> wrote: > >> has_capability() is sometimes needed by modules to test capability >> for specified task other than current, so export it. >> >> Cc: Alex Williamson <alex.williamson@redhat.com> >> Cc: Kirti Wankhede <kwankhede@nvidia.com> >> Signed-off-by: Jike Song <jike.song@intel.com> >> --- >> kernel/capability.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/kernel/capability.c b/kernel/capability.c >> index 4984e1f..e2e198c 100644 >> --- a/kernel/capability.c >> +++ b/kernel/capability.c >> @@ -318,6 +318,7 @@ bool has_capability(struct task_struct *t, int cap) >> { >> return has_ns_capability(t, &init_user_ns, cap); >> } >> +EXPORT_SYMBOL(has_capability); >> >> /** >> * has_ns_capability_noaudit - Does a task have a capability (unaudited) > > Are we using EXPORT_SYMBOL vs EXPORT_SYMBOL_GPL here to match the other > exports in this file? We could use _GPL to match the expected caller > of this. > Yes, I chose EXPORT_SYMBOL to match the existing exports in capability.c. Either is good to me, of course :) > > Serge, > > Do you have any comments on this patch? I'd be happy to pull it > through the vfio tree with an appropriate Ack. Thanks, Guess Serge still on Xmas vocation? :) -- Thanks, Jike
On Wed, Jan 11, 2017 at 01:47:01PM -0500, Alex Williamson wrote:
> On Thu, 22 Dec 2016 00:10:15 +0800
> Jike Song <jike.song@intel.com> wrote:
>
> > has_capability() is sometimes needed by modules to test capability
> > for specified task other than current, so export it.
> >
> > Cc: Alex Williamson <alex.williamson@redhat.com>
> > Cc: Kirti Wankhede <kwankhede@nvidia.com>
> > Signed-off-by: Jike Song <jike.song@intel.com>
> > ---
> > kernel/capability.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/capability.c b/kernel/capability.c
> > index 4984e1f..e2e198c 100644
> > --- a/kernel/capability.c
> > +++ b/kernel/capability.c
> > @@ -318,6 +318,7 @@ bool has_capability(struct task_struct *t, int cap)
> > {
> > return has_ns_capability(t, &init_user_ns, cap);
> > }
> > +EXPORT_SYMBOL(has_capability);
> >
> > /**
> > * has_ns_capability_noaudit - Does a task have a capability (unaudited)
>
> Are we using EXPORT_SYMBOL vs EXPORT_SYMBOL_GPL here to match the other
> exports in this file? We could use _GPL to match the expected caller
> of this.
>
>
> Serge,
>
> Do you have any comments on this patch? I'd be happy to pull it
> through the vfio tree with an appropriate Ack. Thanks,
Sure, thanks, looks good to me.
Acked-by: Serge Hallyn <serge@hallyn.com>