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.

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

pcap_interfaces_map in pmacctd doesn't work with multiple items on same interface #531

Closed
maplewf opened this issue Sep 19, 2021 · 11 comments
Assignees
Labels

Comments

@maplewf
Copy link

maplewf commented Sep 19, 2021

Hi @paololucente,

Here is problem about pmacctd with pcap_interfaces_map configuration. Please see the pcap_interfaces_map as below:

ifindex=100 ifname=eth0 direction=in
ifindex=200 ifname=eth0 direction=out
ifindex=300 ifname=eth1 direction=in
ifindex=400 ifname=eth1 direction=out

With this configuration, pmacctd only can get traffic from eth0 in and eth1 in. After checking source code, there is function called pm_pcap_interfaces_map_getentry_by_ifname as below:

struct pm_pcap_interface *pm_pcap_interfaces_map_getentry_by_ifname(struct pm_pcap_interfaces *map, char *ifname)
{
  int idx;

  for (idx = 0; idx < map->num; idx++) {
    if (strlen(map->list[idx].ifname) == strlen(ifname) && !strncmp(map->list[idx].ifname, ifname, strlen(ifname))) {
      return &map->list[idx];
    }
  }

  return NULL;
}

It always returns the first met value of same ifname in the map. I don't know if it's expected result, but for me, when you want to mark bi-direction of traffic at same time, it's not supported so far unless you start two pmacctd process for each direction.

@paololucente
Copy link
Member

Hi @maplewf ,

Thing is: ULOG/NFLOG (uacctd daemon) is well integrated in the Linux kernel and offers a header where - depending at which point a packet is sampled - one has readily available input and/or output interface.

In case of libpcap (pmacctd daemon), which is portable but not well integrated in the Linux kernel, you have only one single reference point: the interface where a packet is sampled. So you can't really say packet X is ingress interface Y and egress interface Z - which is what you want.

In turn, you may help yourself with pre_tag_map and src_mac / dst_mac (for example) or other primitives there available that may give you a hint of which interface the traffic is ingress / egress.

Paolo

@maplewf
Copy link
Author

maplewf commented Sep 20, 2021

Hi @paololucente,

Please correct me if I'm wrong. ifindex=100 ifname=eth0 direction=in will collect ingress traffic of eth0 via libpcap, it's not used to mark eth0 as ingress interface, but it will fill in_iface field with ifindex for eth0 ingress traffic flow if it's used in aggregate, at least it works in this way in my experience(pmacct v1.7.6), I don't see any problem in here.

What I really concern is that it can't work as expect in following way, so far, it only collect ingress traffic of eth0, ignore egress traffic, vice versa. The reason is already written in first post:

ifindex=100 ifname=eth0 direction=in
ifindex=200 ifname=eth0 direction=out

But you must complain that why not just use ifindex=100 ifname=eth0 to collect bi-direction traffic to walk this around. Because, I want to get my flow/aggregate with direction information in in_iface or out_iface, add direction in map seems the only way to archive that as I know as far.

Of course, I can add ingress and egress configuration in different map and use different pmacctd process to handle them, but don't you think it's too inconvenience? Why not we can't use one map and one process to archive all these things?BTW, in https://github.com/pmacct/pmacct/blob/master/examples/pcap_interfaces.map.example, it doesn't forbid configuration like this.

best regards.

@paololucente
Copy link
Member

paololucente commented Sep 20, 2021

Hi @maplewf ,

I understand the source of confusion & you are right documentation should be improved. Specifying ifname=eth0 direction=in does capture ingress traffic, specifying ifname=eth0 direction=out does capture egress traffic and not specifying any direction, ie. ifname=eth0, does capture both ingress and egress.

Now problem is that doing so, that is specifying ifname=eth0, does disable marking in_iface and out_iface with the ifindex value specified in the pcap_interfaces_map: reason being pmacct can't be sure anymore whether a specific packet is ingress or egress - again all of this is mapping the concept of direction basing on how libpcap works and to make that direction distinction clear, yeah, one should run two daemons, one per direction (or involve tags, ie. pre_tag_map, and helping out with MAC addresses and suchs).

Paolo

@maplewf
Copy link
Author

maplewf commented Sep 22, 2021

Hi @paololucente,

To be honest, I don't really understand what you said pmacct can't be sure anymore whether a specific packet is ingress or egress - again all of this is mapping the concept of direction basing on how libpcap works and to make that direction distinction clear.

In my understanding, the problem is that pmacctd can't read pcap_interface_map correctly by function call pm_pcap_interfaces_map_getentry_by_ifname. Here is my fix:

  -    pm_pcap_if_entry = pm_pcap_interfaces_map_getentry_by_ifname(&pm_pcap_if_map, ifname);
  -    ret = pm_pcap_add_interface(&devices.list[devices.num], ifname, pm_pcap_if_entry, psize);
  +   ret = pm_pcap_add_interface(&devices.list[devices.num], ifname, &pm_pcap_if_map.list[pm_pcap_if_idx-1], psize);

Use map with index directly instead of pm_pcap_interfaces_map_getentry_by_ifname to read every entry in the map, I have compiled and tested, it works and being able to recognize the direction with interface.

That's why I said at first that I don't understand you said pmacctd can't distinguish direction, I think as long as it can read all entries, then it can work perfectly.

But I have to say that this fix doesn't consider the scenario of reloading map, if you think this is right thinking, then maybe it needs to be reconsidered too.

best regards.

paololucente added a commit that referenced this issue Sep 23, 2021
…e the same interface is present multiple times (maybe with different directions). To address Issue #531
@paololucente
Copy link
Member

Hi @maplewf ,

I did review existing code and, indeed, pm_pcap_interfaces_map_getentry_by_ifname() has little role when looping over the interfaces map. It does not even achieve de-duplication in case the same interface is present twice, it just misbehaves.

Thanks for your proposal for a code change. I just committed a piece of code, if you see it's your proposal but polished a bit. Can you give it a try and confirm it working for you?

Paolo

@maplewf
Copy link
Author

maplewf commented Sep 27, 2021

Hi @paololucente,

I have verified the patch, it works! But about this function, I still have one comment:

int pm_pcap_device_getindex_byifname(struct pm_pcap_devices *map, char *ifname)
{
  int loc_idx;
   for (loc_idx = 0; loc_idx < map->num; loc_idx++) {
    if (strlen(map->list[loc_idx].str) == strlen(ifname) && !strncmp(map->list[loc_idx].str, ifname, strlen(ifname))) {
      return loc_idx;
    }
  }

  return ERR;
}

This function is used to reload the interface map, but it still only return the first index of match items. So if there are one more items which has same ifname in the map, then it will miss items in the reload.

paololucente added a commit that referenced this issue Sep 27, 2021
…e the same interface is present multiple times (maybe with different directions). (2) To address Issue #531
@paololucente
Copy link
Member

Hi @maplewf ,

Thanks for spotting this issue, you are right & hopefully my last committed fixed these other bits. Can you please give it a try and confirm? I did test very basic scenarios and it seemed to work.

Paolo

@maplewf
Copy link
Author

maplewf commented Sep 30, 2021

Hi @paololucente,

int pm_pcap_device_getindex_by_ifname_direction(struct pm_pcap_devices *map, char *ifname, int direction)
{
  int loc_idx;
   for (loc_idx = 0; loc_idx < map->num; loc_idx++) {
    if (strlen(map->list[loc_idx].str) == strlen(ifname) && !strncmp(map->list[loc_idx].str, ifname, strlen(ifname))) {
      return loc_idx;
    }
  }
  return ERR;
}

you introduced the direction, but the function never use it..., so it actually nothing changed

paololucente added a commit that referenced this issue Sep 30, 2021
…e the same interface is present multiple times (maybe with different directions). (3) To address Issue #531
@paololucente
Copy link
Member

paololucente commented Sep 30, 2021

Hi @maplewf ,

Third try. Can you give it a try? If it does not work, please feel free to propose a patch too.

Paolo

@maplewf
Copy link
Author

maplewf commented Sep 30, 2021

Hi @paololucente

It's perfect this time. Thanks for great support

@paololucente
Copy link
Member

Wonderfulness @maplewf , thanks for confirming! Paolo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants