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

DPI related bug-fixes #5

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

DPI related bug-fixes #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b56fa43
DPI: remove zero data length check from dpi_ndpi_session_first_packet()
shubham-cdot Jan 13, 2021
42d292f
DPI: use USE_NDPI flag
shubham-cdot Jan 13, 2021
54da769
DPI: use correct loop variable
shubham-cdot Jan 13, 2021
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
2 changes: 1 addition & 1 deletion src/npf/dpi/dpi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ dpi_session_first_packet(struct npf_session *se, struct npf_cache *npc,

free_flows:
for (unsigned int j = 0; j < i; j++) {
struct flow_procs_tup *tup = &flow->flows[i];
struct flow_procs_tup *tup = &flow->flows[j];
if (!tup)
continue;
if (!tup->procs)
Expand Down
6 changes: 3 additions & 3 deletions src/npf/dpi/ndpi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,8 @@ dpi_ndpi_session_flow_destroy(struct dpi_engine_flow *dpi_flow)
static int
dpi_ndpi_session_first_packet(struct npf_session *se __unused,
struct npf_cache *npc __unused, struct rte_mbuf *mbuf,
int dir, uint32_t data_len, struct dpi_engine_flow **dpi_flow)
int dir, uint32_t data_len __unused,
struct dpi_engine_flow **dpi_flow)
{
struct ndpi_flow *flow = zmalloc_aligned(sizeof(struct ndpi_flow));
if (!flow)
Expand All @@ -315,8 +316,7 @@ dpi_ndpi_session_first_packet(struct npf_session *se __unused,
if (!flow->dest_id)
goto dest_id_error;

if (data_len != 0 && !dpi_ndpi_process_pkt(
(struct dpi_engine_flow *)flow, mbuf, dir))
Copy link
Contributor

Choose a reason for hiding this comment

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

@dfawcus

In dpi.c:dpi_get_data_len() there is a comment around where the UDP length is validated, intended to simply cause packets to be dropped if the length is invalid.

The above change would seem to make the call graph to here broken, in that such packets now will not be dropped, so there would need to be another way of dealing with that case.

It is also worth keeping in mind that (eventually) UDP packets will have the possibility of having 0 traditional data, while actually carrying elsewhere because of the UDP-Options draft which is in progress.

It strikes me that the data_length field should maybe be removed from the prototype for this function, as it arose when the mbuf was not passed in, and we passed instead a data+length tuple. It is rather superfluous at the moment, as it can be extracted from the mbuf - see dpi_get_data_length().

if (!dpi_ndpi_process_pkt((struct dpi_engine_flow *)flow, mbuf, dir))
return -EINVAL;

*dpi_flow = (struct dpi_engine_flow *)flow;
Expand Down
9 changes: 7 additions & 2 deletions src/npf/rproc/npf_ext_appfw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,10 +389,15 @@ appfw_action(npf_cache_t *npc, struct rte_mbuf **nbuf, void *arg,
*/
dpi_flow = npf_session_get_dpi(se);
if (!dpi_flow) {
#ifdef USE_NDPI
uint8_t engines[] = { IANA_USER, IANA_NDPI };

size_t engines_len = 2;
#else
uint8_t engines[] = { IANA_USER };
size_t engines_len = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

@dfawcus

The second line would be better outwith of the conditional and written as:

size_t engines_len = ARRAY_SIZE(engines);

Which raises another question, each other call to dpi_session_first_packet() passes an final pair or parameters, being 'engines_length' and 'engines', however 'engines_length' was a constant '2' in each case.

This change implies that something related to that pair should be altered - somehow.

#endif /* USER_NDPI */
Copy link
Contributor

Choose a reason for hiding this comment

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

@dfawcus

Not directly related, but possibly important.

I believe that when the app-fw was created, 0 data-length packets were not fed through it, and so did not count towards the number of initial packets it had to see before converging on an answer. So TCP flags only updates would not count.

The context of this PR implies that this is no longer the case, and it may inform upon why we had issues with the initial count behaviour in the post Qosmos world. So there may be something to be updated in the app-fw now as to which packets it counts towards that initial set - possibly 0-data length TCP packet should not be counted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly copied to the wrong place, this makes more sense in relation to the prior commit.
(I can't recall which commit I added the comment to in the internal PR system).

rc = dpi_session_first_packet(se, npc, *nbuf,
ah->ah_initial_dir, 2, engines);
ah->ah_initial_dir, engines_len, engines);
if (rc != 0)
goto drop;
dpi_flow = npf_session_get_dpi(se);
Expand Down
8 changes: 7 additions & 1 deletion src/npf/rproc/npf_ext_dpi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,9 +178,15 @@ dpi_match(npf_cache_t *npc, struct rte_mbuf *mbuf, const struct ifnet *ifp,
/* Find or attach the DPI flow info. Do first packet inspection */
struct dpi_flow *dpi_flow = npf_session_get_dpi(se);
if (!dpi_flow) {
#ifdef USE_NDPI
uint8_t engines[] = {IANA_USER, IANA_NDPI};
size_t engines_len = 2;
#else
uint8_t engines[] = {IANA_USER};
size_t engines_len = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

@dfawcus

ARRAY_SIZE, as earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this repeated code should be in a little helper function so USE_NDPI is kept to a minimum, and there's only one place to add additional DPI engines in future.

#endif /* USER_NDPI */
int error = dpi_session_first_packet(se, npc, mbuf,
dir, 2, engines);
dir, engines_len, engines);
if (error)
goto drop;
dpi_flow = npf_session_get_dpi(se);
Expand Down
11 changes: 9 additions & 2 deletions src/pipeline/nodes/l3_dpi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,15 @@ ip_dpi_process_common(struct pl_packet *pkt, bool v4, int dir)
}

/* Attach the DPI flow info, do first packet inspection */
uint8_t engines[] = {IANA_USER, IANA_NDPI};
(void)dpi_session_first_packet(se, npc, m, dir, 2, engines);
#ifdef USE_NDPI
uint8_t engines[] = {IANA_USER, IANA_NDPI};
size_t engines_len = 2;
#else
uint8_t engines[] = {IANA_USER};
size_t engines_len = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

@dfawcus

ARRAY_SIZE, as earlier.

#endif /* USER_NDPI */

(void)dpi_session_first_packet(se, npc, m, dir, engines_len, engines);

done:
if (dir == PFIL_IN)
Expand Down