Re: [PATCH net v3] net: l2tp: fix reading optional fields of L2TPv3

[Jacob Wen <jian.w.wen@oracle.com>]

Thanks for the detailed review.

On 1/30/19 6:37 AM, Guillaume Nault wrote:
> On Tue, Jan 29, 2019 at 02:18:13PM +0800, Jacob Wen wrote:
>> Use pskb_may_pull() to make sure the optional fields are in skb linear
>> parts, so we can safely read them in l2tp_recv_common.
>>
> Looks fine to me. Just a few nitpicks. Not sure if they're worth a repost.
> But if you send a v4, you can:
>    * Add the proper Fixes tag.
>    * Drop 'net:' from the subsystem prefix ('l2tp:' is enough).
>    * Move your patch history inside the commit description.
>    * Keep my Acked-by tag.
Done in v4.
>> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
>> index 26f1d435696a..82c28008b438 100644
>> --- a/net/l2tp/l2tp_core.c
>> +++ b/net/l2tp/l2tp_core.c
>> @@ -884,6 +884,10 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
>>   		goto error;
>>   	}
>>   
>> +	if (tunnel->version != L2TP_HDR_VER_2 &&
>>
> Using tunnel->version == L2TP_HDR_VER_3 would have been clearer.
Ditto.
>
>> diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
>> index 9c9afe94d389..870f8ccf95f7 100644
>> --- a/net/l2tp/l2tp_core.h
>> +++ b/net/l2tp/l2tp_core.h
>> @@ -301,6 +301,27 @@ static inline bool l2tp_tunnel_uses_xfrm(const struct l2tp_tunnel *tunnel)
>>   }
>>   #endif
>>   
>> +/* Pull optional fields of L2TPv3 */
>> +static inline int l2tp_v3_pull_opt(struct l2tp_session *session, struct sk_buff *skb,
>>
> The comment and function name are a bit misleading: nothing is pulled
> here.
>
You are right. The misleading is inherited from pskb_may_pull.
s/l2tp_v3_pull_opt/l2tp_v3_ensure_opt_in_linear/ in v4.
>
> BTW, Do you plan to also fix L2TPv2?
> It looks like defining L2TP_HDR_SIZE_MAX to 14 (size of L2TPv2 header
> with all optional fields) and using it in place of L2TP_HDR_SIZE_SEQ in
> l2tp_udp_recv_core() should be enough.
I will do that with your Suggested-by. Thanks.

-- 
Jacob