aya: maps + programs using RawFd could lead to fd leaking

[nrxus]

As of Rust v1.63.0 new types OwnedFd and BorrowedFd were added so that rust can handle the lifetime of file descriptors using normal RAII and borrow rules. I think it'd benefit aya to move towards using these types as it can prevent some unforeseen errors or file descriptor leakage either by aya or by its users.

As a side note on this we may have to remove the implementation of Clone on some types, at the very least on Map and change it to a try_clone(). The current implementation is faulty in that it both ignores errors and the cloned file descriptor is not set to close on exec which afaict should generally be set. While in practice this may not be an issue 99% of the time it is still not good practice to ignore errors during duplication of a file descriptor. Rust's OwnedFd does the right thing on its try_clone() method.

I was contemplating working on this and making a PR but I wanted to make sure this is something that the maintainers of aya are ok merging before I start on it since it'd be a pretty hefty PR and it might have some breaking changes, including having to up the MSRV to v1.63.

To clarify, apart from the map clone issue above I am not claiming that there is currently a leakage or errors in the file descriptor handling. My wanting to move towards OwnedFd/BorrowedFd is from an abundance of caution to prevent possible errors either in the existing code or in the future as RawFds make it easier to have oopsies.

[alessandrod]

I was contemplating working on this and making a PR but I wanted to make sure this is something that the maintainers of aya are ok merging before I start on it since it'd be a pretty hefty PR and it might have some breaking changes, including having to up the MSRV to v1.63.

We've discussed this in the past and is definitely something we want, we just need someone to drive the effort. I think MSRV 1.63 would be fine.

[tamird]

This is almost done. We have two instances of held RawFd left:

https://github.com/aya-rs/aya/blob/0b6ea313ded3240715d1b30d3b247e2bc983659e/aya/src/programs/socket_filter.rs#L126-L127

https://github.com/aya-rs/aya/blob/0b6ea313ded3240715d1b30d3b247e2bc983659e/aya/src/programs/xdp.rs#L234

I'm not exactly sure what the semantics there are supposed to be. @dave-tucker ?

[dave-tucker]

This is almost done. We have two instances of held RawFd left:

https://github.com/aya-rs/aya/blob/0b6ea313ded3240715d1b30d3b247e2bc983659e/aya/src/programs/socket_filter.rs#L126-L127

https://github.com/aya-rs/aya/blob/0b6ea313ded3240715d1b30d3b247e2bc983659e/aya/src/programs/xdp.rs#L234

I'm not exactly sure what the semantics there are supposed to be. @dave-tucker ?

Any LinkId type is just supposed to be a cookie 🍪 we exchange with a user to uniquely identify a specific attachment. This is because Aya chooses to own the FD backing that attachment. It's actually used as the key in the HashMap we use to track links:

https://github.com/aya-rs/aya/blob/0b6ea313ded3240715d1b30d3b247e2bc983659e/aya/src/programs/links.rs#L31-L34

Since it's used as a unique ID, hopefully, we can get rid of the need to use FDs here - i.e cast to i32 and hash together - since at this point they should never be used as FDs again.

However, these Link::Id cookies 🍪 can be used to detach a program: https://github.com/aya-rs/aya/blob/0b6ea313ded3240715d1b30d3b247e2bc983659e/aya/src/programs/xdp.rs#L166-L171

Now comes the fun part.

Link calls detach on drop. So any concrete Link type (NlLink and SocketFilterLink) need to be able to detach themselves. Which of course leads to problems when they require the FD of the program they are attached to..

So TL;DR:

In both cases, when a link is created the kernel refcnt for a bpf program will be > 1. In other words, the program will not disappear until the last link has gone (even if you explicitly unload it) - this might be a feature we can use to our advantage.

For XDP specifically:

• We could borrow the ProgramFd under the same lifetime as the XdpProgram
• Worst case, it's ok to dup and keep an OwnedFd to avoid lifetimes

For SocketFilter:

• No SocketFilterLink can outlive the socket to which it's attached
• sock_fd should be a BorrowedFd
• prog_fd, like XDP, could be borrowed or owned

[tamird]

Sounds good. You interested in making that change?