Fix #166: transition to standard skb linkage inside TempstaFW. #1000

aleksostapenko · 2018-04-04T17:58:48Z

No description provided.

krizhanovsky

The changes required by #166 are much deeper than implemented in the PR.

krizhanovsky · 2018-04-05T15:03:27Z

linux-4.9.35.patch

@@ -1,5 +1,5 @@
 diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt


Please remove the patch from the master and generate a new patch from https://github.com/tempesta-tech/linux-4.14.32-tfw .

krizhanovsky · 2018-04-05T15:06:37Z

tempesta_fw/ss_skb.c

+	nskb->prev = skb;
+	skb->next = nskb;
+	if (nskb->next)
+		nskb->next->prev = nskb;
 }


The function must be removed since it does just the same as standard list_add() except it breaks the standard list behaviour using NULL as a tail pointer. The point of #166 is that our internal skb lists mut be the same as in the kernel.

Our current internal implementation is based and largely tied with NULL-terminated lists, so we cannot use full circular lists without significant redesign of code which work with our internal skb queue.

krizhanovsky · 2018-04-05T15:20:44Z

tempesta_fw/ss_skb.c

@@ -253,11 +250,11 @@ __skb_insert_after(struct sk_buff *skb, struct sk_buff *nskb)
 static inline void
 __skb_skblist_fixup(SsSkbList *skb_list)


I believe we can and should remove SsSkbList now. Regarding __skb_skblist_fixup() the only place where we use it is skb_fragment() and it seems that we used the function just because we had no circular list, so the function seems also must be removed.

SsSkbList is replaced with simple struct sk_buff *skb_head pointer to NULL-terminated list with additional tail pointer (skb_head->prev).

krizhanovsky · 2018-04-05T15:22:39Z

tempesta_fw/ss_skb.c

@@ -236,14 +236,11 @@ __it_next_data(struct sk_buff *skb, int i, TfwStr *it)
 static inline void
 __skb_insert_after(struct sk_buff *skb, struct sk_buff *nskb)


I believe all our own list stuff like ss_skb_queue_head_init(), ss_skb_queue_empty(), ss_skb_queue_tail() and so on must be removed now.

Functions ss_skb_queue_head_init(), ss_skb_queue_empty(), ss_skb_next(), ss_skb_peek() had been removed.

krizhanovsky · 2018-04-05T15:31:37Z

tempesta_fw/ss_skb.h

+
+	skb->next = skb_shinfo(skb)->frag_list;
+	list->first = list->last = skb;
+}


Well, skb_shinfo(skb)->frag_list is just a pointer to skb - the proof that we don't need SsSkbList any more. Since the function will work just with plain skb pointers, just 2 lines in size and is used only once, there is no need to keep the function and the code can be directly mode to ss_skb_unroll().

krizhanovsky · 2018-04-05T15:33:18Z

tempesta_fw/ss_skb.h

-	SsSkbCb *scb = TFW_SKB_CB(skb);
+	skb->prev = list->last;
+	list->last = skb;
+}


The same as above for ss_skb_init_from_frag_list().

krizhanovsky · 2018-04-05T15:34:41Z

tempesta_fw/ss_skb.h

 }

 static inline struct sk_buff *
 ss_skb_next(struct sk_buff *skb)
 {
-	return TFW_SKB_CB(skb)->next;
+	return skb->next;
 }


These changes and at the below (ss_skb_next(), ss_skb_peek_tail() and so on) - please remove the stuff and use standard skbuff and list API.

Some functions had been removed (#1000 (comment)) but we cannot remove all our skb list API, as standard kernel skbuff list API is not applicable for our internal skb queue.

…2 (PR#1000).

krizhanovsky

The patch is much better, but the list design is till inconsistent. At least very strong motivation is requred to leave with the list in current state.

krizhanovsky · 2018-04-09T16:01:32Z

tempesta_fw/ss_skb.c

-	WARN_ON_ONCE(TFW_SKB_CB(skb_list->last)->next);
+	if (last->next)
+		skb_head->prev = last->next;
+	WARN_ON_ONCE(skb_head->prev->next);


I still don't see the reason for the function and why can't we move to double linked list if we need last pointer or accurate NULL terminated list like skb->frag_list: current prev meaning the last item as well as double linked list with NULLs for the last and first items look ugly and error prone.

vankoven

I have nothing to add to @krizhanovsky review. It's odd to have null-terminated double-linked list. Using only one well-known approach is less error-prone.

krizhanovsky

The lists look good for me. Please make the minor cleanups and necessary changes to compile for 4.14.

krizhanovsky · 2018-04-11T17:50:20Z

tempesta_fw/sock.c

+	if (!*skb_head) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}


Better to write as

if (WARN_ON_ONCE(!*skb_head)) return 0;

krizhanovsky · 2018-04-11T18:07:02Z

tempesta_fw/ss_skb.c

-	if (last->next)
-		skb_head->prev = last->next;
-	WARN_ON_ONCE(skb_head->prev->next);
+	nskb->next->prev = nskb->prev->next = nskb;


Bit simplier

nskb->next->prev = skb->next = nskb;

Also comment "Note that the list's pointer to the last item is not updated here.has no sense any more and should be removed. Also it's worh mentioning whyskb_insert()` doens't suite our need and we need our own function.

krizhanovsky · 2018-04-11T19:08:33Z

tempesta_fw/ss_skb.h

 	if (*skb_head == skb)
 		*skb_head = skb->next;
-	else


*skb_head == skb should be in the else branch - there is no sense to make the check after *skb_head = NULL

krizhanovsky

Good to merge after probably the one small fix.

krizhanovsky · 2018-04-13T12:06:27Z

tempesta_fw/filter.c

@@ -298,7 +300,8 @@ tfw_filter_stop(void)
 	if (tfw_runstate_is_reconfig())
 		return;
 	if (ip_filter_db) {
-		nf_unregister_hooks(tfw_nf_ops, ARRAY_SIZE(tfw_nf_ops));
+		nf_unregister_net_hooks(&init_net, tfw_nf_ops,
+					ARRAY_SIZE(tfw_nf_ops));


Why init_net and why don't you add the hooks to all network namespaces as other security modules do this using register_pernet_subsys()?

vankoven

Good to merge after small fixes.

vankoven · 2018-04-13T11:52:36Z

tempesta_fw/cfg.c

@@ -1612,8 +1612,8 @@ tfw_cfg_read_file(const char *path, size_t *file_size)
 	do {
 		TFW_DBG3("read by offset: %d\n", (int)offset);
 		read_size = min((size_t)(buf_size - offset), PAGE_SIZE);
-		bytes_read = vfs_read(fp, out_buf + offset, read_size, \
-				      &offset);
+		bytes_read = kernel_read(fp, out_buf + offset, read_size, \


Backslash is not required here and can be removed.

vankoven · 2018-04-13T12:05:23Z

tempesta_fw/main.c

@@ -341,7 +341,7 @@ static struct ctl_table tfw_sysctl_tbl[] = {
 		.mode		= 0644,
 		.proc_handler	= tfw_ctlfn_state_io,
 	},
-	{ 0 }
+	{}


Although GCC complies the code, the statement is valid for C++ and prohibited for C. { 0 } is more correct for C.

In context of introducing structure randomization functionality into the kernel, various structures (including struct ctl_table) had been marked with __randomize_layout (torvalds/linux@3859a27#diff-9ad17f888d19adc0b5148b288e553b9b) which implies designated_init GCC attribute, which in turn means that initialization of structures declared with this attribute must use designated initializers rather than positional initializers. Also -Werror=designated-init compiler option had been introduced into kernel makefile (torvalds/linux@c834f0e). So when struct ctl_table initialized with
{ 0 }, GCC produces an error, since it recognizes this type of initialization as positional partial initialization (not the designated initialization).

Fix #166: transition to standard skb linkage inside TempstaFW.

550fe48

aleksostapenko requested review from krizhanovsky and vankoven April 4, 2018 17:59

Change patch for linux kernel (PR#1000).

8e14674

aleksostapenko added the ready for review label Apr 4, 2018

krizhanovsky requested changes Apr 5, 2018

View reviewed changes

aleksostapenko added 4 commits April 6, 2018 14:07

Revert kernel patch for linux-4.9.35 and add new one for linux-4.14.3…

2cf77a0

…2 (PR#1000).

Remove kernel patch for linux-4.9.35 (PR#1000).

435c8c7

Rework internal skb list (PR#1000).

14efafb

Add tail pointer into internal skb list (PR#1000).

b2cf1bc

krizhanovsky requested changes Apr 9, 2018

View reviewed changes

vankoven suggested changes Apr 10, 2018

View reviewed changes

Make internal skb list as cyclic doubly linked (PR#1000).

527392f

krizhanovsky requested changes Apr 11, 2018

View reviewed changes

aleksostapenko added 2 commits April 12, 2018 15:07

Corrections according review comments (PR#1000).

f4c37f5

Porting TempestaFW for 4.14.32 kernel (PR#1000).

929063c

krizhanovsky approved these changes Apr 13, 2018

View reviewed changes

vankoven approved these changes Apr 13, 2018

View reviewed changes

Corrections according additional review comments (PR#1000).

a83be28

aleksostapenko merged commit 6de9198 into master Apr 14, 2018

aleksostapenko deleted the ao-166 branch April 14, 2018 07:47

aleksostapenko mentioned this pull request Jul 23, 2018

Tempesta TLS performance optimizations #1037

Merged

krizhanovsky added a commit that referenced this pull request Aug 22, 2018

Fix review commits, the rest of #1000(#166) in particular

329e1c2

krizhanovsky added a commit that referenced this pull request Dec 25, 2018

Fix review commits, the rest of #1000(#166) in particular

18344a2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #166: transition to standard skb linkage inside TempstaFW. #1000

Fix #166: transition to standard skb linkage inside TempstaFW. #1000

aleksostapenko commented Apr 4, 2018

krizhanovsky left a comment

krizhanovsky Apr 5, 2018

aleksostapenko Apr 6, 2018

krizhanovsky Apr 5, 2018

aleksostapenko Apr 9, 2018

krizhanovsky Apr 5, 2018

aleksostapenko Apr 9, 2018

krizhanovsky Apr 5, 2018

aleksostapenko Apr 9, 2018

krizhanovsky Apr 5, 2018

aleksostapenko Apr 9, 2018

krizhanovsky Apr 5, 2018

aleksostapenko Apr 9, 2018

krizhanovsky Apr 5, 2018

aleksostapenko Apr 9, 2018

krizhanovsky left a comment

krizhanovsky Apr 9, 2018

vankoven left a comment •

edited

Loading

krizhanovsky left a comment

krizhanovsky Apr 11, 2018

aleksostapenko Apr 12, 2018

krizhanovsky Apr 11, 2018

aleksostapenko Apr 12, 2018

krizhanovsky Apr 11, 2018

aleksostapenko Apr 12, 2018

krizhanovsky left a comment

krizhanovsky Apr 13, 2018

aleksostapenko Apr 14, 2018

vankoven left a comment

vankoven Apr 13, 2018

aleksostapenko Apr 14, 2018

vankoven Apr 13, 2018

aleksostapenko Apr 14, 2018

		@@ -1,5 +1,5 @@
		diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt

		@@ -253,11 +250,11 @@ __skb_insert_after(struct sk_buff skb, struct sk_buff nskb)
		static inline void
		__skb_skblist_fixup(SsSkbList *skb_list)

		@@ -236,14 +236,11 @@ __it_next_data(struct sk_buff skb, int i, TfwStr it)
		static inline void
		__skb_insert_after(struct sk_buff skb, struct sk_buff nskb)

Fix #166: transition to standard skb linkage inside TempstaFW. #1000

Fix #166: transition to standard skb linkage inside TempstaFW. #1000

Conversation

aleksostapenko commented Apr 4, 2018

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven left a comment • edited Loading

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven left a comment •

edited

Loading