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

Jump to bottom

long tc program names are not truncated, causing a netlink error #610

Open
Sherlock-Holo opened this issue May 23, 2023 · 10 comments · Fixed by pooladkhay/aya#1 · May be fixed by #728
Open

long tc program names are not truncated, causing a netlink error #610

Sherlock-Holo opened this issue May 23, 2023 · 10 comments · Fixed by pooladkhay/aya#1 · May be fixed by #728
Labels
good first issue Good for newcomers

Comments

@Sherlock-Holo
Copy link

when this error message reported, what is the real reason?
I can't attach this Sherlock-Holo/mahiro@fe85104#diff-b47e6e512f3e362859296f1fca2c484e0699ca24db91fcf4813a04af127701a4R20 bpf program, however, I can attach Sherlock-Holo/mahiro@fe85104#diff-b47e6e512f3e362859296f1fca2c484e0699ca24db91fcf4813a04af127701a4R16
@Sherlock-Holo
Copy link
Author

Sherlock-Holo commented May 23, 2023 •
edited

oh... I change #[classifier(name = "dnat_ingress_with_redirect_route")] to #[classifier(name = "dnat_ingress_with_redirect")] and it works
that makes me more confused...

@dave-tucker
Copy link
Member

I would guess the name is too long. Would be interesting where the ENOSPC comes from though.
Either running with strace or Aya's debug logging enabled would be helpful.

@Sherlock-Holo
Copy link
Author

it seems the error reported by this

aya/aya/src/sys/netlink.rs

Line 491 in 58f1ecb

return Err(io::Error::new(io::ErrorKind::Other, "no space left"));

@dave-tucker
Copy link
Member

Ah yeah here's the issue. Name should probably be truncated here:

aya/aya/src/sys/netlink.rs

Line 134 in 58f1ecb

options.write_attr_bytes(TCA_BPF_NAME as u16, prog_name.to_bytes_with_nul())?;

@dave-tucker dave-tucker changed the title tc load report 'no space left' long tc program names are not truncated, causing a netlink error May 26, 2023
@dave-tucker dave-tucker added the good first issue Good for newcomers label May 26, 2023
@pooladkhay
Copy link

According to the kernel code, the name can be up to 256 bytes long:
https://github.com/torvalds/linux/blob/master/net/sched/cls_bpf.c#L28

#define CLS_BPF_NAME_LEN	256

And realised you have set the total length of attributes to 64 bytes:
https://github.com/aya-rs/aya/blob/main/aya/src/sys/netlink.rs#L254

#[repr(C)]
struct TcRequest {
    header: nlmsghdr,
    tc_info: tcmsg,
    attrs: [u8; 64],
}

I increased the length of the attributes and it works fine until the name reaches 256 bytes limit enforced by kernel and this error appears:

Error: netlink error while attaching ebpf program to tc

Caused by:
    Invalid argument (os error 22)

So we can conclude that the limit is enforced by Aya not the kernel.
Are there any particular reasons behind choosing 64 as the length of the attributes ?

@tamird
Copy link
Member

tamird commented Jul 19, 2023

@pooladkhay could you send a patch that increases this limit with a test? I'd be happy to guide you through writing the test.

@pooladkhay
Copy link

pooladkhay commented Jul 20, 2023 •
edited

@pooladkhay could you send a patch that increases this limit with a test? I'd be happy to guide you through writing the test.

@tamird Yeah I'd love to do that,
For the actual size, from what I saw it always requires 33 bytes for values other than name (289 bytes in total) but it just sounds like a magic number. I'll try to figure out the reason behind that 33 and if it can ever increase and will send a patch.

In the meantime I'd really appreciate it if you tell me more about the test.

pooladkhay added a commit to pooladkhay/aya that referenced this issue Jul 31, 2023
@pooladkhay
aya: fix tc name limit
b2a9043 
By increasing the attrs length to a number that can accommodate 256-bytes-long tc names enforced by the kernel, aya no longer limits this attribute.

292 is the smallest possible size which is divisible by 4 (alignment requirement).

Fixes: aya-rs#610
This was referenced Jul 31, 2023
Merged
Closed
pooladkhay added a commit to pooladkhay/aya that referenced this issue Aug 1, 2023
@pooladkhay
aya: fix tc name limit
bf10166 
By increasing the attrs length to a number that can accommodate 256-bytes-long tc names enforced by the kernel, aya no longer limits this attribute.

292 is the smallest possible size which is divisible by 4 (alignment requirement).

Fixes: aya-rs#610
@tamird
Copy link
Member

tamird commented Aug 1, 2023

Ah, sorry I missed your reply. The tests are in the integration-test directory. I think the smoke test is closest to what you're looking for, but have a look around. You can run these tests locally using cargo xtask integration-test -- <my-test-name>.

@tamird
Copy link
Member

tamird commented Aug 1, 2023 •
edited

Actually, this may be the test you're looking for:

aya/test/integration-test/src/tests/load.rs

Line 17 in 445cb8b

fn long_name() {

@pooladkhay
Copy link

@tamird No worries,
Thank you, I'll update the comment and add the test tomorrow.

pooladkhay added a commit to pooladkhay/aya that referenced this issue Aug 8, 2023
@pooladkhay
aya: fix tc name limit
6f63454 
The buffer for attributes should be sized to hold at least 256 bytes, based on `CLS_BPF_NAME_LEN = 256` from the kernel:
https://github.com/torvalds/linux/blob/02aee814/net/sched/cls_bpf.c#L28

This commit also includes integration tests with an eBPF program of type classifier with a 256-byte-long name.

Fixes: aya-rs#610
@pooladkhay pooladkhay linked a pull request Aug 8, 2023 that will close this issue
Open
pooladkhay added a commit to pooladkhay/aya that referenced this issue Aug 12, 2023
@pooladkhay
aya: fix tc name limit
4859705 
The buffer for attributes should be sized to hold at least 256 bytes, based on `CLS_BPF_NAME_LEN = 256` from the kernel:
https://github.com/torvalds/linux/blob/02aee814/net/sched/cls_bpf.c#L28

This commit also includes integration tests with an eBPF program of type classifier with a 256-byte-long name.

Fixes: aya-rs#610

integration-ebpf: fix Cargo.toml formatting
pooladkhay added a commit to pooladkhay/aya that referenced this issue Dec 1, 2023
@pooladkhay
aya: fix tc name limit
9606690 
The buffer for attributes should be sized to hold at least 256 bytes, based on `CLS_BPF_NAME_LEN = 256` from the kernel:
https://github.com/torvalds/linux/blob/02aee814/net/sched/cls_bpf.c#L28

Fixes: aya-rs#610

aya: add netlink_clsact_qdisc_exists function

public-api: add clsact_qdisc_exists

integration-test: add tc_name_limit

integration-test: add tc_name_limit_exceeded

Signed-off-by: Mohammad Javad Pooladkhay <m.pooladkhay@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
4 participants
@tamird @dave-tucker @Sherlock-Holo @pooladkhay