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.
Just my 2ç,
Gerd
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");
>
> sg_ret = &sg_first[rm->m_used_sgs];
> sg_init_table(sg_ret, nents);
>
