[Devel] Re: [PATCH 3/4] Update the UNIX buffer restore code to match the new format saved in the image file

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Dan Smith (danms at us.ibm.com):
>>>> /* Make sure there's room in the send buffer */
>>>> sndbuf = sk->sk_sndbuf;
>>>> -	if (((sk->sk_sndbuf - atomic_read(&sk->sk_wmem_alloc)) < len) &&
>>>> +	if (((sk->sk_sndbuf - atomic_read(&sk->sk_wmem_alloc)) < h->lin_len) &&
>>>> capable(CAP_NET_ADMIN))
>>>> -		sk->sk_sndbuf += len;
>>>> +		sk->sk_sndbuf += h->lin_len;
>>>> else
>> sk-> sk_sndbuf = sysctl_wmem_max;
>>
>> SH> Can you explain what's going on here?
>>
>> If we're trying to restore a buffer that is larger than the remaining
>> space in the buffer, then one of two things can happen:
>>
>> 1. You're privileged and we make the space you need
>> 2. You're not privileged so we give you the benefit of the doubt and
>>    set the buffer limit to the system default
>>
>> In the case of 2, if that system default still isn't enough then the
>> sendmsg() will fail like it normally would.
> 
> But so should check whether h->len_len < sysctl_wmem_max before
> doing the capable check?  Remember that any check for capable()
> will set PF_SUPERPRIV on the task, so it's better to not call it
> if it wasn't definately needed.
> 
>> The reason for this is that the application could have loaded up its
>> legitimate buffer with data and then set the buffer limit low.  That
>> doesn't purge the data it already had buffered, it just limits how
>> much you can add to it.  So, in order to not fail a restart of such a
>> legitimate situation, we assume the system default instead of the
>> limit set by the user.

Maybe also worth beefing up the comment near that code to help
future reviewers/developers...

The patch is good.

Oren

_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers