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
