[rds-devel] [PATCH rds linux-next v2 2/2] net/rds: remove user triggered WARN_ON in rds_sendmsg

[santosh.shilimkar at oracle.com]

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 at oracle.com>
>>
>> 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 at kernel.org>
>> Acked-by: Santosh Shilimkar <santosh.shilimkar at oracle.com>
>> Signed-off-by: shamir rabinovitch <shamir.rabinovitch at oracle.com>
>> ---
>>   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