Hi Jason,
1. When userspace get "-EACCES" value, it shouldn't continue query
within limited times. There's not much log.
2. It's helpful to know the permission problem through kernel logs.
3. I hit one userspace app crash and checked several aspects without
any finding until dynamic trace the kernel to know that why
"permission" is denied in ib_uverbs_write function.
Regards,
Changcheng
On 11:13 Fri 12 Apr, Jason Gunthorpe wrote:
> On Fri, Apr 12, 2019 at 10:08:48PM +0800, Liu, Changcheng wrote:
> > 1. pr_err_once only print once after it's called. However,
> > dmesg output is flushed after long time. It's hard to get
> > this info without occurring anymore. Use pr_err to print
> > the access permission not allow info.
> >
> > 2. unify ib_safe_file_access check implementation.
> >
> > Signed-off-by: Changcheng Liu <changcheng.liu@xxxxxxxxx>
> >
> > diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c
> > index 7541fbaf58a3..2df984f10a2c 100644
> > +++ b/drivers/infiniband/core/ucm.c
> > @@ -1108,11 +1108,9 @@ static ssize_t ib_ucm_write(struct file *filp, const char __user *buf,
> > struct ib_ucm_cmd_hdr hdr;
> > ssize_t result;
> >
> > - if (!ib_safe_file_access(filp)) {
> > - pr_err_once("ucm_write: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
> > - task_tgid_vnr(current), current->comm);
> > - return -EACCES;
> > - }
> > + result = ib_safe_file_access_check(filp);
> > + if (result == -EACCES)
> > + return result;
> >
> > if (len < sizeof(hdr))
> > return -EINVAL;
> > diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
> > index 01d68ed46c1b..7b298ff3e4a5 100644
> > +++ b/drivers/infiniband/core/ucma.c
> > @@ -1664,11 +1664,9 @@ static ssize_t ucma_write(struct file *filp, const char __user *buf,
> > struct rdma_ucm_cmd_hdr hdr;
> > ssize_t ret;
> >
> > - if (!ib_safe_file_access(filp)) {
> > - pr_err_once("ucma_write: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
> > - task_tgid_vnr(current), current->comm);
> > - return -EACCES;
> > - }
> > + ret = ib_safe_file_access_check(filp);
> > + if (ret == -EACCES)
> > + return ret;
> >
> > if (len < sizeof(hdr))
> > return -EINVAL;
> > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> > index 5f366838b7ff..71dc597c9f4e 100644
> > +++ b/drivers/infiniband/core/uverbs_main.c
> > @@ -662,11 +662,9 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> > int srcu_key;
> > ssize_t ret;
> >
> > - if (!ib_safe_file_access(filp)) {
> > - pr_err_once("uverbs_write: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
> > - task_tgid_vnr(current), current->comm);
> > - return -EACCES;
> > - }
> > + ret = ib_safe_file_access_check(filp);
> > + if (ret == -EACCES)
> > + return ret;
> >
> > if (count < sizeof(hdr))
> > return -EINVAL;
> > diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
> > index 78fa634de98a..5ed15dec9cdb 100644
> > +++ b/drivers/infiniband/hw/qib/qib_file_ops.c
> > @@ -2043,11 +2043,9 @@ static ssize_t qib_write(struct file *fp, const char __user *data,
> > ssize_t ret = 0;
> > void *dest;
> >
> > - if (!ib_safe_file_access(fp)) {
> > - pr_err_once("qib_write: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
> > - task_tgid_vnr(current), current->comm);
> > - return -EACCES;
> > - }
> > + ret = ib_safe_file_access_check(fp);
> > + if (ret == -EACCES)
> > + return ret;
> >
> > if (count < sizeof(cmd.type)) {
> > ret = -EINVAL;
> > diff --git a/include/rdma/ib.h b/include/rdma/ib.h
> > index 4f385ec54f80..736c67530107 100644
> > +++ b/include/rdma/ib.h
> > @@ -95,12 +95,19 @@ struct sockaddr_ib {
> > * traditional suid executable error message writes, but also various kernel
> > * interfaces that can write to file descriptors.
> > *
> > - * This function provides protection for the legacy API by restricting the
> > + * This macro provides protection for the legacy API by restricting the
> > * calling context.
> > */
> > -static inline bool ib_safe_file_access(struct file *filp)
> > -{
> > - return filp->f_cred == current_cred() && !uaccess_kernel();
> > -}
> > +#define ib_safe_file_access_check(filp) \
> > +({ \
> > + ssize_t rst = 0; \
> > + if (!filp->f_cred == current_cred() || uaccess_kernel()) { \
> > + pr_err("%s: process %d (%s) changed security contexts " \
> > + "opening file descriptor, it's not allowed.\n", \
> > + __func__, task_tgid_vnr(current), current->comm); \
> > + rst = -EACCES; \
> > + } \
> > + rst; \
> > +})
>
> We can't let user space spam the log.
>
> Jason
