Infiniband: make ehca_pd use struct pid pointer rather than pid_t
PATCH 1/3 Infiniband: make ehca_pd use struct pid pointer rather than pid_t
The task_struct->tgid field is about to become deprecated, due to pid namespaces make tasks have many pids, not one. The infiniband driver is one of the code, that still uses it in some places.
First make struct ehca_pd use a struct pid pointer and consolidate owners check into one call (saves space).
---
drivers/infiniband/hw/ehca/ehca_av.c | 18 +++--------------- drivers/infiniband/hw/ehca/ehca_classes.h | 2 +- drivers/infiniband/hw/ehca/ehca_mrmw.c | 15 +++------------ drivers/infiniband/hw/ehca/ehca_pd.c | 22 ++++++++++++++++------ drivers/infiniband/hw/ehca/ehca_qp.c | 30 +++++------------------------- drivers/infiniband/hw/ehca/ehca_tools.h | 5 +++++ drivers/infiniband/hw/ehca/ehca_uverbs.c | 6 +----- 7 files changed, 34 insertions(+), 64 deletions(-)
drivers/infiniband/hw/ehca/ehca_av.c +3 -15
@@ -173,14 +173,10 @@ int ehca_modify_ah(struct ib_ah *ah, struct ib_ah_attr *ah_attr) struct ehca_pd *my_pd = container_of(ah->pd, struct ehca_pd, ib_pd); struct ehca_shca *shca = container_of(ah->pd->device, struct ehca_shca, ib_device); -u32 cur_pid = current->tgid;if (my_pd->ib_pd.uobject && my_pd->ib_pd.uobject->context && - my_pd->ownpid != cur_pid) { -ehca_err (ah->device,"Invalid caller pid=%x ownpid=%x" , - cur_pid, my_pd->ownpid); +ehca_check_pd_owner (ah->device, my_pd))return -EINVAL ; - }memset (&new_ehca_av,0 ,sizeof (new_ehca_av)); new_ehca_av.sl = ah_attr->sl;
@@ -243,14 +239,10 @@ int ehca_query_ah(struct ib_ah *ah, struct ib_ah_attr *ah_attr) { struct ehca_av *av = container_of(ah, struct ehca_av, ib_ah); struct ehca_pd *my_pd = container_of(ah->pd, struct ehca_pd, ib_pd); -u32 cur_pid = current->tgid;if (my_pd->ib_pd.uobject && my_pd->ib_pd.uobject->context && - my_pd->ownpid != cur_pid) { -ehca_err (ah->device,"Invalid caller pid=%x ownpid=%x" , - cur_pid, my_pd->ownpid); +ehca_check_pd_owner (ah->device, my_pd))return -EINVAL ; - }memcpy (&ah_attr->grh.dgid, &av->av.grh.word_3,sizeof (ah_attr->grh.dgid));
@@ -274,14 +266,10 @@ int ehca_query_ah(struct ib_ah *ah, struct ib_ah_attr *ah_attr)int ehca_destroy_ah (struct ib_ah *ah) { struct ehca_pd *my_pd = container_of(ah->pd, struct ehca_pd, ib_pd); -u32 cur_pid = current->tgid;if (my_pd->ib_pd.uobject && my_pd->ib_pd.uobject->context && - my_pd->ownpid != cur_pid) { -ehca_err (ah->device,"Invalid caller pid=%x ownpid=%x" , - cur_pid, my_pd->ownpid); +ehca_check_pd_owner (ah->device, my_pd))return -EINVAL ; - } kmem_cache_free(av_cache, container_of(ah, struct ehca_av, ib_ah));
drivers/infiniband/hw/ehca/ehca_classes.h +1 -1
@@ -132,7 +132,7 @@ struct ehca_shca {struct ehca_pd {struct ib_pd ib_pd;struct ipz_pd fw_pd; -u32 ownpid; +struct pid *ownpid;/* small queue mgmt */ struct mutex lock;struct list_head free[2 ];
drivers/infiniband/hw/ehca/ehca_mrmw.c +3 -12
@@ -429,12 +429,9 @@ int ehca_rereg_phys_mr(struct ib_mr *mr,u32 num_kpages =0 ;u32 num_hwpages =0 ;struct ehca_mr_pginfo pginfo; -u32 cur_pid = current->tgid;if (my_pd->ib_pd.uobject && my_pd->ib_pd.uobject->context && - (my_pd->ownpid != cur_pid)) { -ehca_err (mr->device,"Invalid caller pid=%x ownpid=%x" , - cur_pid, my_pd->ownpid); +ehca_check_pd_owner (mr->device, my_pd)) { ret = -EINVAL ; goto rereg_phys_mr_exit0; }
@@ -578,14 +575,11 @@ int ehca_query_mr(struct ib_mr *mr, struct ib_mr_attr *mr_attr) container_of(mr->device, struct ehca_shca, ib_device); struct ehca_mr *e_mr = container_of(mr, struct ehca_mr, ib.ib_mr); struct ehca_pd *my_pd = container_of(mr->pd, struct ehca_pd, ib_pd); -u32 cur_pid = current->tgid;unsigned long sl_flags;struct ehca_mr_hipzout_parms hipzout;if (my_pd->ib_pd.uobject && my_pd->ib_pd.uobject->context && - (my_pd->ownpid != cur_pid)) { -ehca_err (mr->device,"Invalid caller pid=%x ownpid=%x" , - cur_pid, my_pd->ownpid); +ehca_check_pd_owner (mr->device, my_pd)) { ret = -EINVAL ; goto query_mr_exit0; }
@@ -635,12 +629,9 @@ int ehca_dereg_mr(struct ib_mr *mr) container_of(mr->device, struct ehca_shca, ib_device); struct ehca_mr *e_mr = container_of(mr, struct ehca_mr, ib.ib_mr); struct ehca_pd *my_pd = container_of(mr->pd, struct ehca_pd, ib_pd); -u32 cur_pid = current->tgid;if (my_pd->ib_pd.uobject && my_pd->ib_pd.uobject->context && - (my_pd->ownpid != cur_pid)) { -ehca_err (mr->device,"Invalid caller pid=%x ownpid=%x" , - cur_pid, my_pd->ownpid); +ehca_check_pd_owner (mr->device, my_pd)) { ret = -EINVAL ; goto dereg_mr_exit0; }
drivers/infiniband/hw/ehca/ehca_pd.c +16 -6
@@ -45,6 +45,19 @@static struct kmem_cache *pd_cache; +int ehca_check_pd_owner (struct ib_device *device,struct ehca_pd *pd) +{ +struct pid *cur_pid; + + cur_pid =task_tgid (current); +if (likely (pd->ownpid == cur_pid)) +return 0 ; + +ehca_err (device,"Invalid caller pid=%x ownpid=%x" , +pid_nr (cur_pid),pid_nr (pd->ownpid)); +return -EINVAL ; +} +struct ib_pd *ehca_alloc_pd (struct ib_device *device,struct ib_ucontext *context,struct ib_udata *udata) {
@@ -58,7 +71,7 @@ struct ib_pd *ehca_alloc_pd(struct ib_device *device,return ERR_PTR (-ENOMEM ); } - pd->ownpid = current->tgid; + pd->ownpid =get_pid (task_tgid (current));for (i =0 ; i <2 ; i++) {INIT_LIST_HEAD (&pd->free[i]);INIT_LIST_HEAD (&pd->full[i]);
@@ -85,17 +98,13 @@ struct ib_pd *ehca_alloc_pd(struct ib_device *device,int ehca_dealloc_pd (struct ib_pd *pd) { -u32 cur_pid = current->tgid; struct ehca_pd *my_pd = container_of(pd, struct ehca_pd, ib_pd);int i, leftovers =0 ;struct ipz_small_queue_page *page, *tmp;if (my_pd->ib_pd.uobject && my_pd->ib_pd.uobject->context && - my_pd->ownpid != cur_pid) { -ehca_err (pd->device,"Invalid caller pid=%x ownpid=%x" , - cur_pid, my_pd->ownpid); +ehca_check_pd_owner (pd->device, my_pd))return -EINVAL ; - }for (i =0 ; i <2 ; i++) {list_splice (&my_pd->full[i], &my_pd->free[i]);
@@ -110,6 +119,7 @@ int ehca_dealloc_pd(struct ib_pd *pd)ehca_warn (pd->device,"Some small queue pages were not freed" ); +put_pid (my_pd->ownpid);kmem_cache_free (pd_cache, my_pd);return 0 ;
drivers/infiniband/hw/ehca/ehca_qp.c +5 -25
@@ -1528,14 +1528,10 @@ int ehca_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr, int attr_mask, struct ehca_qp *my_qp = container_of(ibqp, struct ehca_qp, ib_qp); struct ehca_pd *my_pd = container_of(my_qp->ib_qp.pd, struct ehca_pd, ib_pd); -u32 cur_pid = current->tgid;if (my_pd->ib_pd.uobject && my_pd->ib_pd.uobject->context && - my_pd->ownpid != cur_pid) { -ehca_err (ibqp->pd->device,"Invalid caller pid=%x ownpid=%x" , - cur_pid, my_pd->ownpid); +ehca_check_pd_owner (ibqp->pd->device, my_pd))return -EINVAL ; - }/* The if-block below caches qp_attr to be modified for GSI and SMI * qps during the initialization by ib_mad. When the respective port
@@ -1642,16 +1638,12 @@ int ehca_query_qp(struct ib_qp *qp, ib_device);struct ipz_adapter_handle adapter_handle = shca->ipz_hca_handle;struct hcp_modify_qp_control_block *qpcb; -u32 cur_pid = current->tgid;int cnt, ret =0 ;u64 h_ret;if (my_pd->ib_pd.uobject && my_pd->ib_pd.uobject->context && - my_pd->ownpid != cur_pid) { -ehca_err (qp->device,"Invalid caller pid=%x ownpid=%x" , - cur_pid, my_pd->ownpid); +ehca_check_pd_owner (qp->device, my_pd))return -EINVAL ; - }if (qp_attr_mask &QP_ATTR_QUERY_NOT_SUPPORTED ) { ehca_err(qp->device, "Invalid attribute mask "
@@ -1806,13 +1798,9 @@ int ehca_modify_srq(struct ib_srq *ibsrq, struct ib_srq_attr *attr,u64 h_ret;int ret =0 ; -u32 cur_pid = current->tgid;if (my_pd->ib_pd.uobject && my_pd->ib_pd.uobject->context && - my_pd->ownpid != cur_pid) { -ehca_err (ibsrq->pd->device,"Invalid caller pid=%x ownpid=%x" , - cur_pid, my_pd->ownpid); +ehca_check_pd_owner (ibsrq->pd->device, my_pd))return -EINVAL ; - } mqpcb =ehca_alloc_fw_ctrlblock (GFP_KERNEL );if (!mqpcb) {
@@ -1869,16 +1857,12 @@ int ehca_query_srq(struct ib_srq *srq, struct ib_srq_attr *srq_attr) ib_device);struct ipz_adapter_handle adapter_handle = shca->ipz_hca_handle;struct hcp_modify_qp_control_block *qpcb; -u32 cur_pid = current->tgid;int ret =0 ;u64 h_ret;if (my_pd->ib_pd.uobject && my_pd->ib_pd.uobject->context && - my_pd->ownpid != cur_pid) { -ehca_err (srq->device,"Invalid caller pid=%x ownpid=%x" , - cur_pid, my_pd->ownpid); +ehca_check_pd_owner (srq->device, my_pd))return -EINVAL ; - } qpcb =ehca_alloc_fw_ctrlblock (GFP_KERNEL );if (!qpcb) {
@@ -1919,7 +1903,6 @@ static int internal_destroy_qp(struct ib_device *dev, struct ehca_qp *my_qp, struct ehca_pd *my_pd = container_of(my_qp->ib_qp.pd, struct ehca_pd, ib_pd);struct ehca_sport *sport = &shca->sport[my_qp->init_attr.port_num -1 ]; -u32 cur_pid = current->tgid;u32 qp_num = my_qp->real_qp_num;int ret;u64 h_ret;
@@ -1934,11 +1917,8 @@ static int internal_destroy_qp(struct ib_device *dev, struct ehca_qp *my_qp, "user space qp_num=%x", qp_num);return -EINVAL ; } -if (my_pd->ownpid != cur_pid) { -ehca_err (dev,"Invalid caller pid=%x ownpid=%x" , - cur_pid, my_pd->ownpid); +if (ehca_check_pd_owner (dev, my_pd))return -EINVAL ; - } }if (my_qp->send_cq) {
drivers/infiniband/hw/ehca/ehca_tools.h +5 -0
@@ -154,4 +154,9 @@ extern int ehca_debug_level;/* Converts ehca to ib return code */ int ehca2ib_return_code (u64 ehca_rc); +struct ib_device ; +struct ehca_pd ; + +int ehca_check_pd_owner (struct ib_device *device,struct ehca_pd *pd); +#endif /* EHCA_TOOLS_H */
drivers/infiniband/hw/ehca/ehca_uverbs.c +1 -5
@@ -299,12 +294,8 @@ int ehca_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)return -EINVAL ; pd = container_of(qp->ib_qp.pd, struct ehca_pd, ib_pd); -if (pd->ownpid != cur_pid) { -ehca_err (qp->ib_qp.device, -"Invalid caller pid=%x ownpid=%x" , - cur_pid, pd->ownpid); +if (ehca_check_pd_owner (qp->ib_qp.device, pd))return -ENOMEM ; - } uobject =IS_SRQ (qp) ? qp->ib_srq.uobject : qp->ib_qp.uobject;if (!uobject || uobject->context != context)
Quoted 3 lines
The
task_struct->tgidfield is about to become deprecated, due to pid namespaces make tasks have many pids, not one. The infiniband driver is one of the code, that still uses it in some places.
Looks fine in terms of the changes it makes, but actually it seems that the ehca use of this is completely bogus and the ownership checking should be removed.
The core ib_uverbs module has checks that make sure that objects can only be accessed through the file that they were created through; of course there are tricky ways a file can be passed from one process to another, but I don't think we want to disallow userspace processes from trying to do interesting stuff as long as it doesn't hurt anything.
In other words-- ehca shouldn't be looking at tgids or anything like that at all. If there are missing checks then they should be in the core userspace verbs stuff; but I think what we have is actually OK.
ehca guys, what do you think?
- R.
Hi Roland!
Quoted 14 lines
Quoted 3 lines
The
task_struct->tgidfield is about to become deprecated, due to pid namespaces make tasks have many pids, not one. The infiniband driver is one of the code, that still uses it in some places.Looks fine in terms of the changes it makes, but actually it seems that the ehca use of this is completely bogus and the ownership checking should be removed.
The core
ib_uverbsmodule has checks that make sure that objects can only be accessed through the file that they were created through; of course there are tricky ways a file can be passed from one process to another, but I don't think we want to disallow userspace processes from trying to do interesting stuff as long as it doesn't hurt anything.
Quoted 5 lines
In other words-- ehca shouldn't be looking at tgids or anything like that at all. If there are missing checks then they should be in the core userspace verbs stuff; but I think what we have is actually OK.
ehca guys, what do you think?
Reason for above checking is to prevent a child process releasing a resource that the parent process has created and still wants to use. Do you think that's something we can generalize into ib_core? Regards
Nam
Quoted 8 lines
Quoted 3 lines
Reason for above checking is to prevent a child process releasing a resource that the parent process has created and still wants to use. Do you think that's something we can generalize into
ib_core?Clearly if we want that check then it should be in the core uverbs module. But I'm not sure why we would want that check -- I don't see any realistic scenario where that would cause problems, and it seems at least as likely that the check would break an app that intentionally does something clever.
Right, above checking is based on a very simple policy "creator of a resource is also the owner in term of releasing it" and will not cover other customized patterns. We had a case - believe on sles9, in which a child process manipulated/released resources from parent one, and it was not easy to find the bug.
Wrt/ your actual question: we can remove the tgid stuff from ehca kernel code. When do you expect me to send a patch at latest?
Nam
Quoted 5 lines
Right, above checking is based on a very simple policy "creator of a resource is also the owner in term of releasing it" and will not cover other customized patterns. We had a case - believe on sles9, in which a child process manipulated/released resources from parent one, and it was not easy to find the bug.
Hmm, I can see how that might be ugly. On the other hand it doesn't seem like the unix way for a process not to be able to do something with a file if it has a valid fd.
Quoted 2 lines
Wrt/ your actual question: we can remove the tgid stuff from ehca kernel code. When do you expect me to send a patch at latest?
I don't think it's super-urgent. If you can't do it, say, by the end of this week, then I'll apply Pavel's patch so we don't block his progress on namespace stuff. But I would still like to get a patch to move this out of ehca at some point, so please don't drop it.
If you guys think there is value in having the checks, then please send a patch to add the ownership stuff to the uverbs core and we can argue about it then.
Thanks,
Roland
Quoted 3 lines
Reason for above checking is to prevent a child process releasing a resource that the parent process has created and still wants to use. Do you think that's something we can generalize into
ib_core?
Clearly if we want that check then it should be in the core uverbs module. But I'm not sure why we would want that check -- I don't see any realistic scenario where that would cause problems, and it seems at least as likely that the check would break an app that intentionally does something clever.
- R.