On 12/11/18 1:02 PM, Gerd Rausch wrote:
Hi,
Function "sg_init_table" will overwrite/corrupt the memory
behind "sg_ret", if "nents == 0":
"sg_init_table" calls "sg_init_marker", that does a:
sg_mark_end(&sgl[nents - 1]);
Calling "sg_init_table" with "nents == 0" leads to a rather
undesirable behavior.
And if we assume/know that "nents" is always "> 0", we don't
need the "pr_warn" below.
Please avoid top posting.
On 11/12/2018 06.07, Shamir Rabinovitch wrote:
From: shamir rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
per comment from Leon in rdma mailing list
https://lkml.org/lkml/2018/10/31/312 :
Please don't forget to remove user triggered WARN_ON.
https://lwn.net/Articles/769365/
"Greg Kroah-Hartman raised the problem of core kernel API code that will
use WARN_ON_ONCE() to complain about bad usage; that will not generate
the desired result if WARN_ON_ONCE() is configured to crash the machine.
He was told that the code should just call pr_warn() instead, and that
the called function should return an error in such situations. It was
generally agreed that any WARN_ON() or WARN_ON_ONCE() calls that can be
triggered from user space need to be fixed."
Suggested-by: Leon Romanovsky <leon@xxxxxxxxxx>
Acked-by: Santosh Shilimkar <santosh.shilimkar@xxxxxxxxxx>
Signed-off-by: shamir rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
---
net/rds/message.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/rds/message.c b/net/rds/message.c
index 4b00b1152a5f..c28ad4b111af 100644
--- a/net/rds/message.c
+++ b/net/rds/message.c
@@ -313,11 +313,14 @@ struct scatterlist *rds_message_alloc_sgs(struct rds_message *rm, int nents)
struct scatterlist *sg_first = (struct scatterlist *) &rm[1];
struct scatterlist *sg_ret;
- WARN_ON(rm->m_used_sgs + nents > rm->m_total_sgs);
- WARN_ON(!nents);
-
- if (rm->m_used_sgs + nents > rm->m_total_sgs)
+ if (rm->m_used_sgs + nents > rm->m_total_sgs) {
+ pr_warn("rds: alloc sgs failed! total %d used %d nents %d\n",
+ rm->m_total_sgs, rm->m_used_sgs, nents);
return NULL;
+ }
+
+ if (!nents)
+ pr_warn("rds: alloc sgs failed! nents 0\n");
I believe your are taking about above pr_warn, for nents==0 case.
Its ok to drop the pr_warn but if the behavior leads to
corruption, then lets fail the nents==0 case to instead of
silently proceeding.
Regards,
Santosh
