Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix #1147 : init frag variable before use. #1162

Merged
merged 6 commits into from
Feb 12, 2019
Merged

fix #1147 : init frag variable before use. #1162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
516509c
fix #1147 : init frag variable before use.
vankoven Jan 15, 2019
3ed0fd9
Keep arguments of ss_skb_frag_next syncronized
vankoven Jan 24, 2019
f763b9d
Remove dead stores and dengerous initialisation
vankoven Jan 24, 2019
3b930c1
Remove extra apm update for HM responses
vankoven Feb 6, 2019
dc9558f
Fix ignored tfw_msg_write() error
vankoven Feb 6, 2019
96b8933
Add warning when the ss_skb_frag_next() fails for precalculated message
vankoven Feb 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions tempesta_fw/http.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ tfw_http_prep_redirect(TfwHttpMsg *resp, unsigned short status, TfwStr *rmark,
tfw_http_prep_date(__TFW_STR_CH(&h_common_1, 1)->data);

ret = tfw_msg_write(&it, rh);
ret = tfw_msg_write(&it, &h_common_1);
ret |= tfw_msg_write(&it, &h_common_1);
/*
* HTTP/1.0 may have no host part, so we create relative URI.
* See RFC 1945 9.3 and RFC 7231 7.1.2.
Expand Down Expand Up @@ -2787,8 +2787,6 @@ tfw_http_hm_drop_resp(TfwHttpResp *resp)
TfwHttpReq *req = resp->req;

tfw_connection_unlink_msg(resp->conn);
tfw_apm_update(((TfwServer *)resp->conn->peer)->apmref,
resp->jrxtstamp, resp->jrxtstamp - req->jtxtstamp);
tfw_http_conn_msg_free((TfwHttpMsg *)resp);
tfw_http_msg_free((TfwHttpMsg *)req);
}
Expand Down
48 changes: 26 additions & 22 deletions tempesta_fw/msg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,20 +34,22 @@
int
tfw_msg_write(TfwMsgIter *it, const TfwStr *data)
{
char *p;
const TfwStr *c, *end;
skb_frag_t *frag;
unsigned int c_off = 0, f_size, c_size, f_room, n_copy;

BUG_ON(TFW_STR_DUP(data));
if (WARN_ON_ONCE(it->frag >= skb_shinfo(it->skb)->nr_frags))
return -E2BIG;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of *skb == skb_head in ss_skb_frag_next() we get here skb_head == it->skb and 0 == it->frag; so, condition it->frag >= skb_shinfo(it->skb)->nr_frags) probably will be false here and we continue execute all the following tfw_msg_write() calls without warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scenario you've described may happen on subsequent tfw_msg_write() calls, which is very common case. So yes, the condition will false and we will continue to execute the function. But *skb == skb_head really means that we run out of the spare place in the skb fragments, and no more data can be pushed there. So if we continue to execute the function, we will walk through all the fragments once again and return -E2BIG error.

So there is no errors and the function can be called multiple times without any worries. The tfw_msg_write() function is used only for preallocated skbs, and data size is known before skb allocations, thus data should perfectly fit the it.

The reason for WARN_ON is invalid it state, where is it->frag points to nonexistent fragment.


TFW_STR_FOR_EACH_CHUNK(c, data, end) {
char *p;
unsigned int c_off = 0, c_size, f_room, n_copy;
this_chunk:
if (!frag)
return -E2BIG;

c_size = c->len - c_off;
if (it->frag >= 0) {
frag = &skb_shinfo(it->skb)->frags[it->frag];
unsigned int f_size;
skb_frag_t *frag = &skb_shinfo(it->skb)->frags[it->frag];

Copy link
Contributor

@aleksostapenko aleksostapenko Jan 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that there is a problem with using skb header when allocating more than one skb - in tfw_http_msg_setup() each skb is allocated with a place under the header:

tempesta/tempesta_fw/ss_skb.h

Line 174 in 5631d72

struct sk_buff *skb = alloc_skb(MAX_TCP_HEADER + n, GFP_ATOMIC);

but in tfw_msg_write() only the header of the first skb in list is used (it->frag < 0):

tempesta/tempesta_fw/msg.c

Line 59 in 5631d72

} else {

for subsequent skb it-> frag is always initialized to zero:

tempesta/tempesta_fw/ss_skb.h

Line 132 in 5631d72

*f = 0;

so then we always fall into the it->frag >= 0 branch.
Looks like the current master version of tfw_msg_write() has the same issue.

f_size = skb_frag_size(frag);
f_room = PAGE_SIZE - frag->page_offset - f_size;
p = (char *)skb_frag_address(frag) + f_size;
Expand All @@ -62,24 +64,26 @@ tfw_msg_write(TfwMsgIter *it, const TfwStr *data)

memcpy_fast(p, c->data + c_off, n_copy);

if (c_size < f_room) {
/*
* The chunk fits in the SKB fragment with room
* to spare. Stay in the same SKB fragment, switch
* to next chunk of the string.
*/
c_off = 0;
} else {
frag = ss_skb_frag_next(&it->skb, it->skb_head,
&it->frag);
/*
* The chunk occupied all the spare space in the SKB fragment,
* 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
&& ((c_size != f_room)
|| (c + 1 < end))))
{
return -E2BIG;
}
/*
* If all data from the chunk has been copied,
* then switch to the next chunk. Otherwise,
* stay in the current chunk.
* 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 = 0;
} else {
if (c_size != f_room) {
c_off += n_copy;
goto this_chunk;
}
Expand Down
2 changes: 1 addition & 1 deletion tempesta_fw/ss_skb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,9 @@ ss_skb_frag_next(struct sk_buff **skb, const struct sk_buff *skb_head, int *f)
}

*skb = (*skb)->next;
*f = 0;
if (*skb == skb_head || !skb_shinfo(*skb)->nr_frags)
return NULL;
*f = 0;
return &skb_shinfo(*skb)->frags[0];
}

Expand Down