On 11/12/2018 13.44, santosh.shilimkar@xxxxxxxxxx wrote:
> On 12/11/18 1:41 PM, Gerd Rausch wrote:
>>
>>
>> On 11/12/2018 13.32, santosh.shilimkar@xxxxxxxxxx wrote:
>>
>>>>> --- 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.
>>>
>>
>> Correct.
>>
>> IMHO a BUG_ON(!nents) beats memory corruption any day.
>>
> BUG_ON() isn't desirable since it takes the entire server down for
> one modules misbahvior. So lets just return error back to the
> user and abort the send message path. Thanks for expanding your point.
>
If you don't prevent the memory corruption, chances are you'll panic
much later and it's harder to pinpoint the cause.
And that's the good case.
If you're unlucky enough, you'll end up corrupting valuable data that you
won't notice for a while.
It all boils down to whether or not "nents" is known to always being "> 0".
If we know that, no check is necessary.
If we don't know that, we shouldn't corrupt memory.
Just my 2ç,
Gerd
