[netsniff-ng] Re: [PATCH 4/5] flowtop: Lookup process by dst port too
On 2015-10-26 at 15:33:26 +0100, vkochan wrote:
> On Mon, Oct 26, 2015 at 03:26:59PM +0100, Tobias Klauser wrote:
> > On 2015-10-26 at 14:16:09 +0100, vkochan wrote:
> > > On Mon, Oct 26, 2015 at 01:38:41PM +0100, Tobias Klauser wrote:
> > > > On 2015-10-24 at 16:38:10 +0200, Vadim Kochan wrote:
> > > > > From: Vadim Kochan
> > > > >
> > > > > Perform lookup inode by dst port too if remote traffic represented as
> > > > > src flow, so in case if lookup by src port failed then choose
> > > > > inode matched by dst port.
> > > > >
> > > > > Signed-off-by: Vadim Kochan
> > > > > ---
> > > > > flowtop.c | 37 +
> > > > > 1 file changed, 21 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/flowtop.c b/flowtop.c
> > > > > index 6aa0a6e..f36e8fe 100644
> > > > > --- a/flowtop.c
> > > > > +++ b/flowtop.c
> > > > > @@ -503,40 +503,50 @@ static void walk_processes(struct flow_entry *n)
> > > > > closedir(dir);
> > > > > }
> > > > >
> > > > > -static int get_port_inode(uint16_t port, int proto, bool is_ip6)
> > > > > +static void flow_entry_find_process(struct flow_entry *n)
> > > > > {
> > > > > - int ret = -ENOENT;
> > > > > + int src_inode = 0, dst_inode = 0;
> > > > > char path[128], buff[1024];
> > > > > FILE *proc;
> > > > >
> > > > > memset(path, 0, sizeof(path));
> > > > > snprintf(path, sizeof(path), "/proc/net/%s%s",
> > > > > - l4proto2str[proto], is_ip6 ? "6" : "");
> > > > > + l4proto2str[n->l4_proto], n->l3_proto == AF_INET6 ?
> > > > > "6" : "");
> > > > >
> > > > > proc = fopen(path, "r");
> > > > > if (!proc)
> > > > > - return -EIO;
> > > > > + return;
> > > > >
> > > > > + /* Here we try to find process's socket inode by src port, at
> > > > > the same
> > > > > + * time we try to do it by dst port too which will be choosen
> > > > > in case
> > > > > + * if src port inode will be not found, this is needed in case
> > > > > if the
> > > > > + * 1st flow's packet will be originated from the remote server
> > > > > so then
> > > > > + * local host will be represented as dst flow.
> > > > > + */
> > > > > memset(buff, 0, sizeof(buff));
> > > > > -
> > > > > while (fgets(buff, sizeof(buff), proc) != NULL) {
> > > > > - int inode = 0;
> > > > > unsigned int lport = 0;
> > > > > + int inode = 0;
> > > > >
> > > > > buff[sizeof(buff) - 1] = 0;
> > > > > if (sscanf(buff, "%*u: %*X:%X %*X:%*X %*X %*X:%*X
> > > > > %*X:%*X "
> > > > > "%*X %*u %*u %u", &lport, &inode) == 2) {
> > > > > - if ((uint16_t) lport == port) {
> > > > > - ret = inode;
> > > > > +
> > > > > + if ((uint16_t) lport == n->port_src) {
> > > > > + src_inode = inode;
> > > > > break;
> > > > > + } else if ((uint16_t) lport == n->port_dst) {
> > > > > + dst_inode = inode;
> > > >
> > > > Shouldn't we break here as well?
> > > Let me think again on this, I did not put break here because there might
> > > be a case that
> > > port_dst might be matched to some other local port earlier than port_src
> > > and then the wrong inode might found.
> > > But now I see that it can't be solved by this patch too ... Seems this
> > > is not easy to find locally generated flow (except by matching with
> > > local interface addresses) because in case if 1st flow's packet will be
> > > generated from remote, then local traffic will be identified as dst flow
> > > ...
> >
> > Ah, I see.
> >
> > Before you think too much about it: I'd prefer to keep the lookup logic
> > rather simple. So if you can't figure out the inode reliably without
> > keeping a lot of internal state or querying a lot of information (i.e.
> > easily), I'd suggest we just keep the logic as it is and leave dst port
> > lookup aside.
>
> Let me clarify, so your suggestion is to keep current logic with
> additional lookup by port_dst in case if lookup by port_src was failed ?
Nope, I'd suggest we just use the port_src lookup for now since you
pointed out that the solution you propose in this patch might not work
correctly in all cases (i.e. I won't apply the patch).
And only add logic for port_dst lookup which works reliably in any case
without adding too much overhead.
--
You received this message because you are subscribed to the Google Groups
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[netsniff-ng] Re: [PATCH 4/5] flowtop: Lookup process by dst port too
On 2015-10-26 at 14:16:09 +0100, vkochan wrote:
> On Mon, Oct 26, 2015 at 01:38:41PM +0100, Tobias Klauser wrote:
> > On 2015-10-24 at 16:38:10 +0200, Vadim Kochan wrote:
> > > From: Vadim Kochan
> > >
> > > Perform lookup inode by dst port too if remote traffic represented as
> > > src flow, so in case if lookup by src port failed then choose
> > > inode matched by dst port.
> > >
> > > Signed-off-by: Vadim Kochan
> > > ---
> > > flowtop.c | 37 +
> > > 1 file changed, 21 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/flowtop.c b/flowtop.c
> > > index 6aa0a6e..f36e8fe 100644
> > > --- a/flowtop.c
> > > +++ b/flowtop.c
> > > @@ -503,40 +503,50 @@ static void walk_processes(struct flow_entry *n)
> > > closedir(dir);
> > > }
> > >
> > > -static int get_port_inode(uint16_t port, int proto, bool is_ip6)
> > > +static void flow_entry_find_process(struct flow_entry *n)
> > > {
> > > - int ret = -ENOENT;
> > > + int src_inode = 0, dst_inode = 0;
> > > char path[128], buff[1024];
> > > FILE *proc;
> > >
> > > memset(path, 0, sizeof(path));
> > > snprintf(path, sizeof(path), "/proc/net/%s%s",
> > > - l4proto2str[proto], is_ip6 ? "6" : "");
> > > + l4proto2str[n->l4_proto], n->l3_proto == AF_INET6 ? "6" : "");
> > >
> > > proc = fopen(path, "r");
> > > if (!proc)
> > > - return -EIO;
> > > + return;
> > >
> > > + /* Here we try to find process's socket inode by src port, at the same
> > > + * time we try to do it by dst port too which will be choosen in case
> > > + * if src port inode will be not found, this is needed in case if the
> > > + * 1st flow's packet will be originated from the remote server so then
> > > + * local host will be represented as dst flow.
> > > + */
> > > memset(buff, 0, sizeof(buff));
> > > -
> > > while (fgets(buff, sizeof(buff), proc) != NULL) {
> > > - int inode = 0;
> > > unsigned int lport = 0;
> > > + int inode = 0;
> > >
> > > buff[sizeof(buff) - 1] = 0;
> > > if (sscanf(buff, "%*u: %*X:%X %*X:%*X %*X %*X:%*X %*X:%*X "
> > > "%*X %*u %*u %u", &lport, &inode) == 2) {
> > > - if ((uint16_t) lport == port) {
> > > - ret = inode;
> > > +
> > > + if ((uint16_t) lport == n->port_src) {
> > > + src_inode = inode;
> > > break;
> > > + } else if ((uint16_t) lport == n->port_dst) {
> > > + dst_inode = inode;
> >
> > Shouldn't we break here as well?
> Let me think again on this, I did not put break here because there might be
> a case that
> port_dst might be matched to some other local port earlier than port_src and
> then the wrong inode might found.
> But now I see that it can't be solved by this patch too ... Seems this
> is not easy to find locally generated flow (except by matching with
> local interface addresses) because in case if 1st flow's packet will be
> generated from remote, then local traffic will be identified as dst flow ...
Ah, I see.
Before you think too much about it: I'd prefer to keep the lookup logic
rather simple. So if you can't figure out the inode reliably without
keeping a lot of internal state or querying a lot of information (i.e.
easily), I'd suggest we just keep the logic as it is and leave dst port
lookup aside.
--
You received this message because you are subscribed to the Google Groups
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[netsniff-ng] Re: [PATCH 4/5] flowtop: Lookup process by dst port too
On Mon, Oct 26, 2015 at 03:26:59PM +0100, Tobias Klauser wrote:
> On 2015-10-26 at 14:16:09 +0100, vkochan wrote:
> > On Mon, Oct 26, 2015 at 01:38:41PM +0100, Tobias Klauser wrote:
> > > On 2015-10-24 at 16:38:10 +0200, Vadim Kochan wrote:
> > > > From: Vadim Kochan
> > > >
> > > > Perform lookup inode by dst port too if remote traffic represented as
> > > > src flow, so in case if lookup by src port failed then choose
> > > > inode matched by dst port.
> > > >
> > > > Signed-off-by: Vadim Kochan
> > > > ---
> > > > flowtop.c | 37 +
> > > > 1 file changed, 21 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/flowtop.c b/flowtop.c
> > > > index 6aa0a6e..f36e8fe 100644
> > > > --- a/flowtop.c
> > > > +++ b/flowtop.c
> > > > @@ -503,40 +503,50 @@ static void walk_processes(struct flow_entry *n)
> > > > closedir(dir);
> > > > }
> > > >
> > > > -static int get_port_inode(uint16_t port, int proto, bool is_ip6)
> > > > +static void flow_entry_find_process(struct flow_entry *n)
> > > > {
> > > > - int ret = -ENOENT;
> > > > + int src_inode = 0, dst_inode = 0;
> > > > char path[128], buff[1024];
> > > > FILE *proc;
> > > >
> > > > memset(path, 0, sizeof(path));
> > > > snprintf(path, sizeof(path), "/proc/net/%s%s",
> > > > -l4proto2str[proto], is_ip6 ? "6" : "");
> > > > +l4proto2str[n->l4_proto], n->l3_proto == AF_INET6 ?
> > > > "6" : "");
> > > >
> > > > proc = fopen(path, "r");
> > > > if (!proc)
> > > > - return -EIO;
> > > > + return;
> > > >
> > > > + /* Here we try to find process's socket inode by src port, at
> > > > the same
> > > > +* time we try to do it by dst port too which will be choosen
> > > > in case
> > > > +* if src port inode will be not found, this is needed in case
> > > > if the
> > > > +* 1st flow's packet will be originated from the remote server
> > > > so then
> > > > +* local host will be represented as dst flow.
> > > > +*/
> > > > memset(buff, 0, sizeof(buff));
> > > > -
> > > > while (fgets(buff, sizeof(buff), proc) != NULL) {
> > > > - int inode = 0;
> > > > unsigned int lport = 0;
> > > > + int inode = 0;
> > > >
> > > > buff[sizeof(buff) - 1] = 0;
> > > > if (sscanf(buff, "%*u: %*X:%X %*X:%*X %*X %*X:%*X
> > > > %*X:%*X "
> > > >"%*X %*u %*u %u", &lport, &inode) == 2) {
> > > > - if ((uint16_t) lport == port) {
> > > > - ret = inode;
> > > > +
> > > > + if ((uint16_t) lport == n->port_src) {
> > > > + src_inode = inode;
> > > > break;
> > > > + } else if ((uint16_t) lport == n->port_dst) {
> > > > + dst_inode = inode;
> > >
> > > Shouldn't we break here as well?
> > Let me think again on this, I did not put break here because there might
> > be a case that
> > port_dst might be matched to some other local port earlier than port_src
> > and then the wrong inode might found.
> > But now I see that it can't be solved by this patch too ... Seems this
> > is not easy to find locally generated flow (except by matching with
> > local interface addresses) because in case if 1st flow's packet will be
> > generated from remote, then local traffic will be identified as dst flow
> > ...
>
> Ah, I see.
>
> Before you think too much about it: I'd prefer to keep the lookup logic
> rather simple. So if you can't figure out the inode reliably without
> keeping a lot of internal state or querying a lot of information (i.e.
> easily), I'd suggest we just keep the logic as it is and leave dst port
> lookup aside.
Let me clarify, so your suggestion is to keep current logic with
additional lookup by port_dst in case if lookup by port_src was failed ?
Regards,
--
You received this message because you are subscribed to the Google Groups
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[netsniff-ng] Re: [PATCH 4/5] flowtop: Lookup process by dst port too
On Mon, Oct 26, 2015 at 01:38:41PM +0100, Tobias Klauser wrote:
> On 2015-10-24 at 16:38:10 +0200, Vadim Kochan wrote:
> > From: Vadim Kochan
> >
> > Perform lookup inode by dst port too if remote traffic represented as
> > src flow, so in case if lookup by src port failed then choose
> > inode matched by dst port.
> >
> > Signed-off-by: Vadim Kochan
> > ---
> > flowtop.c | 37 +
> > 1 file changed, 21 insertions(+), 16 deletions(-)
> >
> > diff --git a/flowtop.c b/flowtop.c
> > index 6aa0a6e..f36e8fe 100644
> > --- a/flowtop.c
> > +++ b/flowtop.c
> > @@ -503,40 +503,50 @@ static void walk_processes(struct flow_entry *n)
> > closedir(dir);
> > }
> >
> > -static int get_port_inode(uint16_t port, int proto, bool is_ip6)
> > +static void flow_entry_find_process(struct flow_entry *n)
> > {
> > - int ret = -ENOENT;
> > + int src_inode = 0, dst_inode = 0;
> > char path[128], buff[1024];
> > FILE *proc;
> >
> > memset(path, 0, sizeof(path));
> > snprintf(path, sizeof(path), "/proc/net/%s%s",
> > -l4proto2str[proto], is_ip6 ? "6" : "");
> > +l4proto2str[n->l4_proto], n->l3_proto == AF_INET6 ? "6" : "");
> >
> > proc = fopen(path, "r");
> > if (!proc)
> > - return -EIO;
> > + return;
> >
> > + /* Here we try to find process's socket inode by src port, at the same
> > +* time we try to do it by dst port too which will be choosen in case
> > +* if src port inode will be not found, this is needed in case if the
> > +* 1st flow's packet will be originated from the remote server so then
> > +* local host will be represented as dst flow.
> > +*/
> > memset(buff, 0, sizeof(buff));
> > -
> > while (fgets(buff, sizeof(buff), proc) != NULL) {
> > - int inode = 0;
> > unsigned int lport = 0;
> > + int inode = 0;
> >
> > buff[sizeof(buff) - 1] = 0;
> > if (sscanf(buff, "%*u: %*X:%X %*X:%*X %*X %*X:%*X %*X:%*X "
> >"%*X %*u %*u %u", &lport, &inode) == 2) {
> > - if ((uint16_t) lport == port) {
> > - ret = inode;
> > +
> > + if ((uint16_t) lport == n->port_src) {
> > + src_inode = inode;
> > break;
> > + } else if ((uint16_t) lport == n->port_dst) {
> > + dst_inode = inode;
>
> Shouldn't we break here as well?
Let me think again on this, I did not put break here because there might be a
case that
port_dst might be matched to some other local port earlier than port_src and
then the wrong inode might found.
But now I see that it can't be solved by this patch too ... Seems this
is not easy to find locally generated flow (except by matching with
local interface addresses) because in case if 1st flow's packet will be
generated from remote, then local traffic will be identified as dst flow ...
Regards,
--
You received this message because you are subscribed to the Google Groups
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[netsniff-ng] Re: [PATCH 4/5] flowtop: Lookup process by dst port too
On 2015-10-24 at 16:38:10 +0200, Vadim Kochan wrote:
> From: Vadim Kochan
>
> Perform lookup inode by dst port too if remote traffic represented as
> src flow, so in case if lookup by src port failed then choose
> inode matched by dst port.
>
> Signed-off-by: Vadim Kochan
> ---
> flowtop.c | 37 +
> 1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/flowtop.c b/flowtop.c
> index 6aa0a6e..f36e8fe 100644
> --- a/flowtop.c
> +++ b/flowtop.c
> @@ -503,40 +503,50 @@ static void walk_processes(struct flow_entry *n)
> closedir(dir);
> }
>
> -static int get_port_inode(uint16_t port, int proto, bool is_ip6)
> +static void flow_entry_find_process(struct flow_entry *n)
> {
> - int ret = -ENOENT;
> + int src_inode = 0, dst_inode = 0;
> char path[128], buff[1024];
> FILE *proc;
>
> memset(path, 0, sizeof(path));
> snprintf(path, sizeof(path), "/proc/net/%s%s",
> - l4proto2str[proto], is_ip6 ? "6" : "");
> + l4proto2str[n->l4_proto], n->l3_proto == AF_INET6 ? "6" : "");
>
> proc = fopen(path, "r");
> if (!proc)
> - return -EIO;
> + return;
>
> + /* Here we try to find process's socket inode by src port, at the same
> + * time we try to do it by dst port too which will be choosen in case
> + * if src port inode will be not found, this is needed in case if the
> + * 1st flow's packet will be originated from the remote server so then
> + * local host will be represented as dst flow.
> + */
> memset(buff, 0, sizeof(buff));
> -
> while (fgets(buff, sizeof(buff), proc) != NULL) {
> - int inode = 0;
> unsigned int lport = 0;
> + int inode = 0;
>
> buff[sizeof(buff) - 1] = 0;
> if (sscanf(buff, "%*u: %*X:%X %*X:%*X %*X %*X:%*X %*X:%*X "
> "%*X %*u %*u %u", &lport, &inode) == 2) {
> - if ((uint16_t) lport == port) {
> - ret = inode;
> +
> + if ((uint16_t) lport == n->port_src) {
> + src_inode = inode;
> break;
> + } else if ((uint16_t) lport == n->port_dst) {
> + dst_inode = inode;
Shouldn't we break here as well?
--
You received this message because you are subscribed to the Google Groups
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
