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

Sendto pktinfo #101

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

Conversation

mdavidsaver
Copy link
Member

Attempts to address this situation encountered with p4p.gw epics-base/p4p#162 where a PVA client host is intentionally reachable by routes from multiple interfaces of a gateway host. Because PVXS binds UDP sockets to 0.0.0.0 and [::], a SEARCH reply will currently be sent by the most direct route, which may not be the one by which the original SEARCH request arrived.

This PR changes from sendto() to sendmsg() with a IP_PKTINFO or IPV6_PKTINFO control message specifying the requested outbound interface and/or source address.

The source address for a message forwarded by local multicast is taken from the ORIGIN_TAG message, when available and when an interface address. An interface index is looked up from this.

Currently neither override is specified for non-unicast SEARCH replies, nor for BEACON.

This PR also changes the criteria for identifying a UDP message which has been forwarded by a local peer, even when a ORIGIN_TAG is not present. This is done by looking at OS provided interface and destination address, which I think can be trusted. This should also make it possible to distinguish traffic arriving over the loopback which has not be forwarded.

wrt. portability. All major IP stacks I looked at (BSD, Linux, Windows) provide IPV6_PKTINFO. IP_PKTINFO is provided by Linux and Windows, but not BSD. So for RTEMS or OSX, a call to sendmsg() will be equivalent to sendto().

@kasemir @FreddieAkeroyd fyi.

wrapper sendmsg() and WSASendMsg()

Linux and Windows support IPv4 IP_PKTINFO.
BSD, Linux, and Windows support IPV6_PKTINFO

So far RTEMS and OSX, the extra sendto() overrides
will be ignored.
@mdavidsaver
Copy link
Member Author

While I hope this PR will be a practical no-op for the vast majority of hosts, I see a significant chance of some serious regression or interoperability issue. So this PR will likely "age" a bit until I can do (or receive) some test results with the various other PVA implementations (core.pva, pvAccessCPP/Java).

So far I have done some interop testing with phoebus.

Comment on lines +263 to +266
origin_t origin = Direct;
if(manager->ifmap.is_lo(rx.dstif) && dest.compare(lo_mcast_addr.addr,false)==0) {
// packet forwarded by a local PVA peer (maybe us) as IPv4 local multicast
origin = Forward;
Copy link
Member Author

Choose a reason for hiding this comment

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

@FreddieAkeroyd This is the change to detect a "forwarded" packet, arriving via a loopback interface, with the (multicast) destination address of 224.0.0.128. This is distinct from eg. a packet arriving by loopback destined for 127.0.0.1 or 127.255.255.255.

With this PR, it is possible to achieve the same effect as the Linux specific 127.255.255.255 in a portable way.

EPICS_PVA_ADDR_LIST=127.0.0.1 \
EPICS_PVA_AUTO_ADDR_LIST=NO \
  ./bin/linux-x86_64/pvxget from:ioc1 from:ioc2

However, this will need some interop testing to make sure that other implementations treat this situation sensibly.

Better test of whether received packet was forwarded,
based on OS provided meta-data instead of peer provided
unicast flag.

Also use ORIGIN_TAG (original destination) address as
UDP source address if a local interface address.
@mdavidsaver mdavidsaver self-assigned this Feb 17, 2025
@mdavidsaver mdavidsaver added the enhancement New feature or request label Feb 17, 2025
@FreddieAkeroyd
Copy link

Hi @mdavidsaver i tried this with a 127.255.255.255 address list and see messages like WARN pvxs.udp.io Ignore originated from 127.255.255.255 TT reported to the client and in all IOC logs, with 127.0.0.1 I get no warning. Prior to the change i had no warnings with 127.255.255.255 address list. In all cases I get the PV read OK. With this PR change should I have no need to ever use 127.255.255.255 for searches?

@mdavidsaver
Copy link
Member Author

Hi @mdavidsaver i tried this with a 127.255.255.255 address list

Could you try 127.0.0.1 in the address list?

With this PR change should I have no need to ever use 127.255.255.255 for searches?

I don't think so.

@mdavidsaver
Copy link
Member Author

While pvxlist works fine, pvlist (from pvAccessCPP) does not.

It looks like pvlist is incorrectly sending a broadcast search with the Unicast flag set. sigh...

2025-02-19T18:38:35.647184195 WARN pvxs.udp.io Ignore originated from 10.127.127.255 TT
2025-02-19T18:38:35.987015530 WARN pvxs.udp.io Ignore originated from 10.127.127.255 TT

pvlist-unicast

@FreddieAkeroyd
Copy link

FreddieAkeroyd commented Feb 20, 2025

With 127.0.0.1 address list:

pvxget works fine

pvget works but i get the following printed in the ioc log each time i run it
ERR pvxs.tcp.io connection to Client 127.0.0.1:55743 closed with socket error 10054 : An existing connection was forcibly closed by the remote host.

pvlist, pvxlist work fine, no warning/error messages; - however just after printing all its output pvxlist pauses for about 5 second before giving the prompt back whereas pvlist gives the prompt back immediately after output

With 127.255.255.255 address list:

pvget works with same socket close error as above appearing in ioc log

pvxget prints WARN pvxs.udp.io Ignore originated from 127.255.255.255 TT at both client window and all running iocs, but reads value ok

pvlists results in a WARN pvxs.udp.io Ignore originated from 127.255.255.255 TT in server logs (issue you mention above with incorrect unicast flag i guess?)

pvxlist is like pvxget with warning at both client and ioc windows; the same 5 second pause as for 127.0.0.1 is present

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

Successfully merging this pull request may close these issues.

2 participants