On 01/26/2017 02:02 PM, walter harms wrote:
>
>
> Am 26.01.2017 12:23, schrieb Ursula Braun:
>>
>> On 01/26/2017 10:05 AM, Dan Carpenter wrote:
>>> Say we got really unlucky and these failed on the last iteration, then
>>> it could lead to a use after free bug.
>> thanks for reporting this! I had already a similar patch prepared, but not
>> yet submitted. It contains all your added lines plus these additional
>> pre-initializations at definition time:
>>
>> @@ -509,7 +509,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
>> struct smc_connection *conn = &smc->conn;
>> struct smc_link_group *lgr = conn->lgr;
>> int tmp_bufsize, tmp_bufsize_short;
>> - struct smc_buf_desc *sndbuf_desc;
>> + struct smc_buf_desc *sndbuf_desc = NULL;
>> int rc;
>>
>> /* use socket send buffer size (w/o overhead) as start value */
>>
>> @@ -573,7 +575,7 @@ int smc_rmb_create(struct smc_sock *smc)
>> struct smc_connection *conn = &smc->conn;
>> struct smc_link_group *lgr = conn->lgr;
>> int tmp_bufsize, tmp_bufsize_short;
>> - struct smc_buf_desc *rmb_desc;
>> + struct smc_buf_desc *rmb_desc = NULL;
>> int rc;
>>
>> /* use socket recv buffer size (w/o overhead) as start value */
>>
>> If you do not contradict, I will post my enhanced patch version.
>
>
> Independend of that ...
>
> would it be possible to make sndbuf_desc->cpu_addr a zero size array
> and then simply do a:
> sndbuf_desc = kzalloc(sizeof(*sndbuf_desc)+tmp_bufsize, GFP_KERNEL);
>
The idea had been to keep DMA-mapping memory separate from other memory.
But I will experiment with your idea.
> (read also: is the whole list of __GPF* really needed ?)
Currently SMC-R tries to get large chunks of real consecutive memory
initially and lowers this requirement if not available.
We have future work in mind to implement a scattered solution, but
for now we accept initially failing memory allocations, and do not
want these sometimes likely failings to be noisy.
>
> that would make the memhandling more easy
>
> re,
> wh
>
>>>
>>> Fixes: cd6851f30386 ("smc: remote memory buffers (RMBs)")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>>>
>>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>>> index 8b1d34378829..941279e1504e 100644
>>> --- a/net/smc/smc_core.c
>>> +++ b/net/smc/smc_core.c
>>> @@ -535,6 +535,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
>>> /* if send buffer allocation has failed,
>>> * try a smaller one
>>> */
>>> + sndbuf_desc = NULL;
>>> continue;
>>> }
>>> rc = smc_ib_buf_map(lgr->lnk[SMC_SINGLE_LINK].smcibdev,
>>> @@ -543,6 +544,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
>>> if (rc) {
>>> kfree(sndbuf_desc->cpu_addr);
>>> kfree(sndbuf_desc);
>>> + sndbuf_desc = NULL;
>>> continue; /* if mapping failed, try smaller one */
>>> }
>>> sndbuf_desc->used = 1;
>>> @@ -599,6 +601,7 @@ int smc_rmb_create(struct smc_sock *smc)
>>> /* if RMB allocation has failed,
>>> * try a smaller one
>>> */
>>> + rmb_desc = NULL;
>>> continue;
>>> }
>>> rc = smc_ib_buf_map(lgr->lnk[SMC_SINGLE_LINK].smcibdev,
>>> @@ -607,6 +610,7 @@ int smc_rmb_create(struct smc_sock *smc)
>>> if (rc) {
>>> kfree(rmb_desc->cpu_addr);
>>> kfree(rmb_desc);
>>> + rmb_desc = NULL;
>>> continue; /* if mapping failed, try smaller one */
>>> }
>>> rc = smc_ib_get_memory_region(lgr->lnk[SMC_SINGLE_LINK].roce_pd,
>>> @@ -619,6 +623,7 @@ int smc_rmb_create(struct smc_sock *smc)
>>> DMA_FROM_DEVICE);
>>> kfree(rmb_desc->cpu_addr);
>>> kfree(rmb_desc);
>>> + rmb_desc = NULL;
>>> continue;
>>> }
>>> rmb_desc->used = 1;
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
