From 417e3a1e9becbc1076481604a240099b84f719b8 Mon Sep 17 00:00:00 2001 From: Ivan Koveshnikov Date: Mon, 11 Mar 2019 18:31:35 +0500 Subject: [PATCH 1/4] fix #1189: correctly fill header part of skb when writing data into paged fragments ss_skb_frag_next() is always work with TfwMsgIter, so renamed and moved into msg.h, the whole TfwMsgIter became invalid if the function fails. --- tempesta_fw/http_msg.c | 96 ++++++++++++++++-------------------------- tempesta_fw/msg.c | 5 +-- tempesta_fw/msg.h | 17 ++++++++ tempesta_fw/ss_skb.h | 15 ------- 4 files changed, 55 insertions(+), 78 deletions(-) diff --git a/tempesta_fw/http_msg.c b/tempesta_fw/http_msg.c index 26204e7b8..253550d0a 100644 --- a/tempesta_fw/http_msg.c +++ b/tempesta_fw/http_msg.c @@ -889,42 +889,6 @@ tfw_http_msg_setup(TfwHttpMsg *hm, TfwMsgIter *it, size_t data_len) } EXPORT_SYMBOL(tfw_http_msg_setup); -static int -__http_msg_add_data_frag(TfwMsgIter *it, TfwHttpMsg *hm, TfwStr *field, - const TfwStr *data, unsigned int off) -{ - char *p; - skb_frag_t *frag = &skb_shinfo(it->skb)->frags[it->frag]; - unsigned int f_size, d_size, f_room, n_copy; - -next_frag: - if (!frag) - return -E2BIG; - - d_size = data->len - off; - f_size = skb_frag_size(frag); - f_room = PAGE_SIZE - frag->page_offset - f_size; - n_copy = min(d_size, f_room); - if (!n_copy) - return 0; - - p = (char *)skb_frag_address(frag) + f_size; - memcpy_fast(p, data->data + off, n_copy); - skb_frag_size_add(frag, n_copy); - ss_skb_adjust_data_len(it->skb, n_copy); - - if (__tfw_http_msg_add_str_data(hm, field, p, n_copy, it->skb)) - return -ENOMEM; - - if (d_size > f_room) { - frag = ss_skb_frag_next(&it->skb, it->skb_head, &it->frag); - off += n_copy; - goto next_frag; - } - - return 0; -} - /** * Similar to tfw_msg_write(), but properly maintain @hm header * fields, so that @hm can be used in regular transformations. However, @@ -935,34 +899,48 @@ int tfw_http_msg_add_data(TfwMsgIter *it, TfwHttpMsg *hm, TfwStr *field, const TfwStr *data) { - char *p; - unsigned int n_copy; - - WARN_ON_ONCE(TFW_STR_DUP(data)); - WARN_ON_ONCE(!TFW_STR_PLAIN(data)); + unsigned int c_off = 0, c_size; - n_copy = min(data->len, (unsigned long)skb_tailroom(it->skb)); - if (!n_copy) - return 0; - - if (it->frag >= 0) - goto add_frag; - - p = skb_put(it->skb, n_copy); - memcpy_fast(p, data->data, n_copy); + if (WARN_ON_ONCE(TFW_STR_DUP(data) || !TFW_STR_PLAIN(data))) + return -EINVAL; + if (WARN_ON_ONCE(it->frag >= skb_shinfo(it->skb)->nr_frags)) + return -E2BIG; - if (__tfw_http_msg_add_str_data(hm, field, p, n_copy, it->skb)) - return -ENOMEM; + while ((c_size = data->len - c_off)) { + char *p; + unsigned int n_copy, f_room; + + if (it->frag >= 0) { + unsigned int f_size; + skb_frag_t *frag = &skb_shinfo(it->skb)->frags[it->frag]; + + f_size = skb_frag_size(frag); + f_room = PAGE_SIZE - frag->page_offset - f_size; + p = (char *)skb_frag_address(frag) + f_size; + n_copy = min(c_size, f_room); + skb_frag_size_add(frag, n_copy); + ss_skb_adjust_data_len(it->skb, n_copy); + } else { + f_room = skb_tailroom(it->skb); + n_copy = min(c_size, f_room); + p = skb_put(it->skb, n_copy); + } - if (data->len == n_copy) { - if (!skb_tailroom(it->skb)) - it->frag = 0; - return 0; + memcpy_fast(p, data->data, n_copy); + if (__tfw_http_msg_add_str_data(hm, field, p, n_copy, it->skb)) + return -ENOMEM; + c_off += n_copy; + + if (c_size >= f_room) { + if (WARN_ON_ONCE(tfw_msg_iter_next_data_frag(it) + && (c_size != f_room))) + { + return -E2BIG; + } + } } -add_frag: - it->frag = 0; - return __http_msg_add_data_frag(it, hm, field, data, n_copy); + return 0; } void diff --git a/tempesta_fw/msg.c b/tempesta_fw/msg.c index 6ad3c2505..9c016b3e4 100644 --- a/tempesta_fw/msg.c +++ b/tempesta_fw/msg.c @@ -69,10 +69,7 @@ tfw_msg_write(TfwMsgIter *it, const TfwStr *data) * switch to the next fragment. */ if (c_size >= f_room) { - skb_frag_t *frag = ss_skb_frag_next(&it->skb, - it->skb_head, - &it->frag); - if (WARN_ON_ONCE(!frag + if (WARN_ON_ONCE(tfw_msg_iter_next_data_frag(it) && ((c_size != f_room) || (c + 1 < end)))) { diff --git a/tempesta_fw/msg.h b/tempesta_fw/msg.h index d8abba8ac..8b2041621 100644 --- a/tempesta_fw/msg.h +++ b/tempesta_fw/msg.h @@ -65,4 +65,21 @@ int tfw_msg_write(TfwMsgIter *it, const TfwStr *data); int tfw_msg_iter_setup(TfwMsgIter *it, struct sk_buff **skb_head, size_t data_len); +static inline int +tfw_msg_iter_next_data_frag(TfwMsgIter *it) +{ + if (skb_shinfo(it->skb)->nr_frags > it->frag + 1) { + ++it->frag; + return 0; + } + + it->skb = it->skb->next; + if (it->skb == it->skb_head || !skb_shinfo(it->skb)->nr_frags) { + it->frag = MAX_SKB_FRAGS; + return -EINVAL; + } + it->frag = -1; + return 0; +} + #endif /* __TFW_MSG_H__ */ diff --git a/tempesta_fw/ss_skb.h b/tempesta_fw/ss_skb.h index 8e207ad7e..847d1c157 100644 --- a/tempesta_fw/ss_skb.h +++ b/tempesta_fw/ss_skb.h @@ -120,21 +120,6 @@ ss_skb_adjust_data_len(struct sk_buff *skb, int delta) skb->truesize += delta; } -static inline skb_frag_t * -ss_skb_frag_next(struct sk_buff **skb, const struct sk_buff *skb_head, int *f) -{ - if (skb_shinfo(*skb)->nr_frags > *f + 1) { - ++*f; - return &skb_shinfo(*skb)->frags[*f]; - } - - *skb = (*skb)->next; - *f = 0; - if (*skb == skb_head || !skb_shinfo(*skb)->nr_frags) - return NULL; - return &skb_shinfo(*skb)->frags[0]; -} - /* * skb_tailroom - number of bytes at buffer end * From 6b5e2c09c1f1cc67c151ccb1a890a6aa2375376c Mon Sep 17 00:00:00 2001 From: Ivan Koveshnikov Date: Thu, 14 Mar 2019 17:51:02 +0500 Subject: [PATCH 2/4] fix #1210: correctly update message iterator, when a new skb is added * add a new function, to allocate and add a new empty skb to current message iterator. * fix dereferences of invalid pointers in tfw_cache_build_resp_body(): it->frag may be equal to -1. --- tempesta_fw/cache.c | 28 +++++++++++++++++++++------- tempesta_fw/msg.c | 23 +++++++++++++++++++++++ tempesta_fw/msg.h | 2 ++ 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/tempesta_fw/cache.c b/tempesta_fw/cache.c index fb9050798..66438bfaa 100644 --- a/tempesta_fw/cache.c +++ b/tempesta_fw/cache.c @@ -1444,19 +1444,33 @@ tfw_cache_build_resp_body(TDB *db, TfwHttpResp *resp, TdbVRec *trec, TfwMsgIter *it, char *p) { int off, f_size, r; - skb_frag_t *frag; - BUG_ON(!it->skb); - frag = &skb_shinfo(it->skb)->frags[it->frag]; - if (skb_frag_size(frag)) - ++it->frag; + if (WARN_ON_ONCE(!it->skb)) + return -EINVAL; + /* + * If headers perfectly fit allocated skbs, then + * it->skb == it->skb_head, see tfw_msg_iter_next_data_frag(). + * Normally all the headers fit single skb, but these two situations + * can't be distingueshed. Start after last fragment of last skb in list. + */ + if ((it->skb == it->skb_head) || (it->frag == -1)) { + it->skb = ss_skb_peek_tail(&it->skb_head); + it->frag = skb_shinfo(it->skb)->nr_frags; + } + else { + skb_frag_t *frag = &skb_shinfo(it->skb)->frags[it->frag]; + if (skb_frag_size(frag)) + ++it->frag; + } + BUG_ON(it->frag < 0); + if (it->frag >= MAX_SKB_FRAGS - 1 - && (r = tfw_http_msg_setup((TfwHttpMsg *)resp, it, 0))) + && (r = tfw_msg_iter_append_skb(it))) return r; while (1) { if (it->frag == MAX_SKB_FRAGS - && (r = tfw_http_msg_setup((TfwHttpMsg *)resp, it, 0))) + && (r = tfw_msg_iter_append_skb(it))) return r; /* TDB keeps data by pages and we can reuse the pages. */ diff --git a/tempesta_fw/msg.c b/tempesta_fw/msg.c index 9c016b3e4..a53358984 100644 --- a/tempesta_fw/msg.c +++ b/tempesta_fw/msg.c @@ -91,6 +91,11 @@ tfw_msg_write(TfwMsgIter *it, const TfwStr *data) } EXPORT_SYMBOL(tfw_msg_write); +/** + * Allocate list of skbs to store data with given length @data_len and + * initialise the iterator it. Shouldn't be called against previously used + * iterator, since its current state is to be rewritten. + */ int tfw_msg_iter_setup(TfwMsgIter *it, struct sk_buff **skb_head, size_t data_len) { @@ -105,3 +110,21 @@ tfw_msg_iter_setup(TfwMsgIter *it, struct sk_buff **skb_head, size_t data_len) return 0; } + +/** + * Allocate and add a single empty skb (with a place for TCP headers though) + * to the iterator. The allocated skb has no space for the data, user is + * expected to add new paged fragments. + */ +int +tfw_msg_iter_append_skb(TfwMsgIter *it) +{ + int r; + + if ((r = ss_skb_alloc_data(&it->skb_head, 0))) + return r; + it->skb = ss_skb_peek_tail(&it->skb_head); + it->frag = 0; + + return 0; +} diff --git a/tempesta_fw/msg.h b/tempesta_fw/msg.h index 8b2041621..d7f4d0bb9 100644 --- a/tempesta_fw/msg.h +++ b/tempesta_fw/msg.h @@ -64,6 +64,7 @@ typedef struct { int tfw_msg_write(TfwMsgIter *it, const TfwStr *data); int tfw_msg_iter_setup(TfwMsgIter *it, struct sk_buff **skb_head, size_t data_len); +int tfw_msg_iter_append_skb(TfwMsgIter *it); static inline int tfw_msg_iter_next_data_frag(TfwMsgIter *it) @@ -79,6 +80,7 @@ tfw_msg_iter_next_data_frag(TfwMsgIter *it) return -EINVAL; } it->frag = -1; + return 0; } From 4b8f5ff8bff63e8fc8d79b3d6fdcee477d4e7982 Mon Sep 17 00:00:00 2001 From: Ivan Koveshnikov Date: Tue, 19 Mar 2019 19:57:50 +0500 Subject: [PATCH 3/4] msg.c/http_msg.c: remove copy-paste --- tempesta_fw/http_msg.c | 45 ++++++++++++++++++++++----------- tempesta_fw/msg.c | 56 ++---------------------------------------- 2 files changed, 33 insertions(+), 68 deletions(-) diff --git a/tempesta_fw/http_msg.c b/tempesta_fw/http_msg.c index 253550d0a..311835386 100644 --- a/tempesta_fw/http_msg.c +++ b/tempesta_fw/http_msg.c @@ -890,26 +890,26 @@ tfw_http_msg_setup(TfwHttpMsg *hm, TfwMsgIter *it, size_t data_len) EXPORT_SYMBOL(tfw_http_msg_setup); /** - * Similar to tfw_msg_write(), but properly maintain @hm header - * fields, so that @hm can be used in regular transformations. However, - * the header name and the value are not split into different chunks, - * so advanced headers matching is not available for @hm. + * Fill up an HTTP message by iterator @it with data from string @data. + * Properly maintain @hm header @field, so that @hm can be used in regular + * transformations. However, the header name and the value are not split into + * different chunks, so advanced headers matching is not available for @hm. */ int tfw_http_msg_add_data(TfwMsgIter *it, TfwHttpMsg *hm, TfwStr *field, const TfwStr *data) { - unsigned int c_off = 0, c_size; + const TfwStr *c, *end; - if (WARN_ON_ONCE(TFW_STR_DUP(data) || !TFW_STR_PLAIN(data))) - return -EINVAL; + BUG_ON(TFW_STR_DUP(data)); if (WARN_ON_ONCE(it->frag >= skb_shinfo(it->skb)->nr_frags)) return -E2BIG; - while ((c_size = data->len - c_off)) { + TFW_STR_FOR_EACH_CHUNK(c, data, end) { char *p; - unsigned int n_copy, f_room; - + unsigned int c_off = 0, c_size, f_room, n_copy; +this_chunk: + c_size = c->len - c_off; if (it->frag >= 0) { unsigned int f_size; skb_frag_t *frag = &skb_shinfo(it->skb)->frags[it->frag]; @@ -926,17 +926,34 @@ tfw_http_msg_add_data(TfwMsgIter *it, TfwHttpMsg *hm, TfwStr *field, p = skb_put(it->skb, n_copy); } - memcpy_fast(p, data->data, n_copy); - if (__tfw_http_msg_add_str_data(hm, field, p, n_copy, it->skb)) + memcpy_fast(p, c->data + c_off, n_copy); + if (field && n_copy + && __tfw_http_msg_add_str_data(hm, field, p, n_copy, + it->skb)) + { return -ENOMEM; - c_off += n_copy; + } + /* + * The chunk occupied all the spare space in the SKB fragment, + * switch to the next fragment. + */ if (c_size >= f_room) { if (WARN_ON_ONCE(tfw_msg_iter_next_data_frag(it) - && (c_size != f_room))) + && ((c_size != f_room) + || (c + 1 < end)))) { return -E2BIG; } + /* + * Not all data from the chunk has been copied, + * stay in the current chunk and copy the rest to the + * next fragment. + */ + if (c_size != f_room) { + c_off += n_copy; + goto this_chunk; + } } } diff --git a/tempesta_fw/msg.c b/tempesta_fw/msg.c index a53358984..68a2447b5 100644 --- a/tempesta_fw/msg.c +++ b/tempesta_fw/msg.c @@ -19,6 +19,7 @@ */ #include "lib/str.h" #include "msg.h" +#include "http_msg.h" /** * Fill up an HTTP message by iterator @it with data from string @data. @@ -34,60 +35,7 @@ int tfw_msg_write(TfwMsgIter *it, const TfwStr *data) { - const TfwStr *c, *end; - - BUG_ON(TFW_STR_DUP(data)); - if (WARN_ON_ONCE(it->frag >= skb_shinfo(it->skb)->nr_frags)) - return -E2BIG; - - TFW_STR_FOR_EACH_CHUNK(c, data, end) { - char *p; - unsigned int c_off = 0, c_size, f_room, n_copy; -this_chunk: - - c_size = c->len - c_off; - if (it->frag >= 0) { - unsigned int f_size; - skb_frag_t *frag = &skb_shinfo(it->skb)->frags[it->frag]; - - f_size = skb_frag_size(frag); - f_room = PAGE_SIZE - frag->page_offset - f_size; - p = (char *)skb_frag_address(frag) + f_size; - n_copy = min(c_size, f_room); - skb_frag_size_add(frag, n_copy); - ss_skb_adjust_data_len(it->skb, n_copy); - } else { - f_room = skb_tailroom(it->skb); - n_copy = min(c_size, f_room); - p = skb_put(it->skb, n_copy); - } - - memcpy_fast(p, c->data + c_off, n_copy); - - /* - * The chunk occupied all the spare space in the SKB fragment, - * switch to the next fragment. - */ - if (c_size >= f_room) { - if (WARN_ON_ONCE(tfw_msg_iter_next_data_frag(it) - && ((c_size != f_room) - || (c + 1 < end)))) - { - return -E2BIG; - } - /* - * Not all data from the chunk has been copied, - * stay in the current chunk and copy the rest to the - * next fragment. - */ - if (c_size != f_room) { - c_off += n_copy; - goto this_chunk; - } - } - } - - return 0; + return tfw_http_msg_add_data(it, NULL, NULL, data); } EXPORT_SYMBOL(tfw_msg_write); From 2fec8162faee635b0583a79f2ef49fd8a4ca8646 Mon Sep 17 00:00:00 2001 From: Ivan Koveshnikov Date: Wed, 27 Mar 2019 01:02:19 +0500 Subject: [PATCH 4/4] fix misspelling --- tempesta_fw/cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tempesta_fw/cache.c b/tempesta_fw/cache.c index 66438bfaa..300b244aa 100644 --- a/tempesta_fw/cache.c +++ b/tempesta_fw/cache.c @@ -1451,7 +1451,7 @@ tfw_cache_build_resp_body(TDB *db, TfwHttpResp *resp, TdbVRec *trec, * If headers perfectly fit allocated skbs, then * it->skb == it->skb_head, see tfw_msg_iter_next_data_frag(). * Normally all the headers fit single skb, but these two situations - * can't be distingueshed. Start after last fragment of last skb in list. + * can't be distinguished. Start after last fragment of last skb in list. */ if ((it->skb == it->skb_head) || (it->frag == -1)) { it->skb = ss_skb_peek_tail(&it->skb_head);