svn commit: r336282 - head/sys/netinet
Sean Bruno
sbruno at freebsd.org
Sat Jul 14 16:57:34 UTC 2018
On 07/14/18 10:37, John Baldwin wrote:
> On 7/14/18 9:19 AM, Sean Bruno wrote:
>> Author: sbruno
>> Date: Sat Jul 14 16:19:46 2018
>> New Revision: 336282
>> URL: https://svnweb.freebsd.org/changeset/base/336282
>>
>> Log:
>> Fixup memory management for fetching options in ip_ctloutput()
>>
>> Submitted by: Jason Eggleston <jason at eggnet.com>
>> Sponsored by: Limelight Networks
>> Differential Revision: https://reviews.freebsd.org/D14621
>>
>> Modified:
>> head/sys/netinet/ip_output.c
>>
>> Modified: head/sys/netinet/ip_output.c
>> ==============================================================================
>> --- head/sys/netinet/ip_output.c Sat Jul 14 16:06:53 2018 (r336281)
>> +++ head/sys/netinet/ip_output.c Sat Jul 14 16:19:46 2018 (r336282)
>> @@ -1256,12 +1256,18 @@ ip_ctloutput(struct socket *so, struct sockopt *sopt)
>> switch (sopt->sopt_name) {
>> case IP_OPTIONS:
>> case IP_RETOPTS:
>> - if (inp->inp_options)
>> + if (inp->inp_options) {
>> + unsigned long len = ulmin(inp->inp_options->m_len, sopt->sopt_valsize);
>> + struct mbuf *options = malloc(len, M_TEMP, M_WAITOK);
>> + INP_RLOCK(inp);
>> + bcopy(inp->inp_options, options, len);
>> + INP_RUNLOCK(inp);
>> error = sooptcopyout(sopt,
>> - mtod(inp->inp_options,
>> + mtod(options,
>> char *),
>> - inp->inp_options->m_len);
>> - else
>> + len);
>> + free(options, M_TEMP);
>> + } else
>> sopt->sopt_valsize = 0;
>> break;
>
> You can't malloc an mbuf, and you don't really want an mbuf here anyway.
> Also, style(9) doesn't normally assign values when a variable is created.
> I would perhaps have done something like this:
>
> if (inp->inp_options) {
> void *options;
> u_long len;
>
> INP_RLOCK(inp);
> len = ulmin(inp->inp_options->m_len, sopt->sopt_valsize);
> INP_RUNLOCK(inp);
> options = malloc(len, M_TEMP, M_WAITOK);
> INP_RLOCK(inp);
> len = ulmin(inp->inp_options->m_len, len);
> m_copydata(inp->inp_options, 0, len, options);
> INP_RUNLOCK(inp);
> error = sooptcopyout(sopt, options, len);
> free(options, M_TEMP);
> }
>
> The current code isn't doing what you think it is doing since the bcopy()
> copies the m_data pointer from 'inp_options' into 'options' so that
> 'options->m_data' points at the m_data buffer in the 'inp_options' mbuf.
> Thus, the mtod() returns a pointer into 'inp_options' just like the old
> code, so this commit doesn't change anything in terms of the previous
> race (it is still just as broken).
>
> Also, while INP_RLOCK isn't helpful in my version above for reading the
> 'm_len' member (it's racy to read and then malloc no matter what), you
> still need the lock to ensure the inp_options pointer itself is valid
> when you dereference it. Without the lock another thread might have
> freed inp_options out from under you and the unlocked dereference could
> panic (though it probably just reads garbage on most of our architectures
> rather than panicking since we don't tend to invalidate KVA if you lose
> that race).
>
Shall I revert and rethink?
sean
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 618 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20180714/0d409328/attachment.sig>
More information about the svn-src-all
mailing list