diff mbox

[v2] infiniband: remove WARN that is not kernel bug

Message ID 20161121173820.GC23083@leon.nu
State New
Headers show

Commit Message

Leon Romanovsky Nov. 21, 2016, 5:38 p.m. UTC 
On Mon, Nov 21, 2016 at 09:52:53AM -0700, Jason Gunthorpe wrote:
> On Mon, Nov 21, 2016 at 02:14:08PM +0200, Leon Romanovsky wrote:

> > >

> > > In ib_ucm_write function there is a wrong prefix:

> > >

> > > + pr_err_once("ucm_write: process %d (%s) tried to do something hinky\n",

> >

> > I did it intentionally to have the same errors for all flows.

>

> Lets actually use a good message too please?

>

>  pr_err_once("ucm_write: process %d (%s) changed security contexts after opening FD, this is not allowed.\n",

>

> Jason

Comments

Doug Ledford Dec. 14, 2016, 6:16 p.m. UTC | #1 
On 11/21/2016 12:38 PM, Leon Romanovsky wrote:
> On Mon, Nov 21, 2016 at 09:52:53AM -0700, Jason Gunthorpe wrote:

>> On Mon, Nov 21, 2016 at 02:14:08PM +0200, Leon Romanovsky wrote:

>>>>

>>>> In ib_ucm_write function there is a wrong prefix:

>>>>

>>>> + pr_err_once("ucm_write: process %d (%s) tried to do something hinky\n",

>>>

>>> I did it intentionally to have the same errors for all flows.

>>

>> Lets actually use a good message too please?

>>

>>  pr_err_once("ucm_write: process %d (%s) changed security contexts after opening FD, this is not allowed.\n",

>>

>> Jason


I applied Leon's reworked version of this patch, thanks.

-- 
Doug Ledford <dledford@redhat.com>
    GPG Key ID: 0E572FDD
Leon Romanovsky Dec. 14, 2016, 6:27 p.m. UTC | #2 
On Wed, Dec 14, 2016 at 01:16:45PM -0500, Doug Ledford wrote:
> On 11/21/2016 12:38 PM, Leon Romanovsky wrote:

> > On Mon, Nov 21, 2016 at 09:52:53AM -0700, Jason Gunthorpe wrote:

> >> On Mon, Nov 21, 2016 at 02:14:08PM +0200, Leon Romanovsky wrote:

> >>>>

> >>>> In ib_ucm_write function there is a wrong prefix:

> >>>>

> >>>> + pr_err_once("ucm_write: process %d (%s) tried to do something hinky\n",

> >>>

> >>> I did it intentionally to have the same errors for all flows.

> >>

> >> Lets actually use a good message too please?

> >>

> >>  pr_err_once("ucm_write: process %d (%s) changed security contexts after opening FD, this is not allowed.\n",

> >>

> >> Jason

>

> I applied Leon's reworked version of this patch, thanks.


Thanks Doug,
I already forgot about it :)

>

> --

> Doug Ledford <dledford@redhat.com>

>     GPG Key ID: 0E572FDD

>
diff mbox

Patch

From 70f95b2d35aea42e5b97e7d27ab2f4e8effcbe67 Mon Sep 17 00:00:00 2001
From: Leon Romanovsky <leonro@mellanox.com>
Date: Mon, 21 Nov 2016 13:30:59 +0200
Subject: [PATCH rdma-next V2] IB/{core, qib}: Remove WARN that is not kernel bug

WARNINGs mean kernel bugs, in this case, they are placed
to mark programming errors and/or malicious attempts.

BUG/WARNs that are not kernel bugs hinder automated testing efforts.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>

---
 * Improved error prints.
---
 drivers/infiniband/core/ucm.c            | 5 ++++-
 drivers/infiniband/core/ucma.c           | 5 ++++-
 drivers/infiniband/core/uverbs_main.c    | 5 ++++-
 drivers/infiniband/hw/qib/qib_file_ops.c | 5 ++++-
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c
index 7713ef0..579f9a7 100644
--- a/drivers/infiniband/core/ucm.c
+++ b/drivers/infiniband/core/ucm.c
@@ -1104,8 +1104,11 @@  static ssize_t ib_ucm_write(struct file *filp, const char __user *buf,
 	struct ib_ucm_cmd_hdr hdr;
 	ssize_t result;

-	if (WARN_ON_ONCE(!ib_safe_file_access(filp)))
+	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;
+	}

 	if (len < sizeof(hdr))
 		return -EINVAL;
diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
index 9520154..e12f8fa 100644
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -1584,8 +1584,11 @@  static ssize_t ucma_write(struct file *filp, const char __user *buf,
 	struct rdma_ucm_cmd_hdr hdr;
 	ssize_t ret;

-	if (WARN_ON_ONCE(!ib_safe_file_access(filp)))
+	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;
+	}

 	if (len < sizeof(hdr))
 		return -EINVAL;
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 44b1104..38c79ad 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -746,8 +746,11 @@  static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	int srcu_key;
 	ssize_t ret;

-	if (WARN_ON_ONCE(!ib_safe_file_access(filp)))
+	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;
+	}

 	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 382466a..2d1eacf 100644
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -2066,8 +2066,11 @@  static ssize_t qib_write(struct file *fp, const char __user *data,
 	ssize_t ret = 0;
 	void *dest;

-	if (WARN_ON_ONCE(!ib_safe_file_access(fp)))
+	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;
+	}

 	if (count < sizeof(cmd.type)) {
 		ret = -EINVAL;
--
2.7.4