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

fix: build on v6.6+ kernel #415

Merged
merged 1 commit into from
Nov 17, 2023
Merged

fix: build on v6.6+ kernel #415

merged 1 commit into from
Nov 17, 2023

Conversation

iam-TJ
Copy link

@iam-TJ iam-TJ commented Nov 6, 2023

Commit bffcc6882a "genetlink: remove userhdr from struct genl_info" caused the build to fail since the field no longer exists.

Replace with run-time calculation of the header offset.

@ydahhrk
Copy link
Member

ydahhrk commented Nov 10, 2023

I'm sorry for the silence. I'll review and likely merge next Saturday.

@ydahhrk
Copy link
Member

ydahhrk commented Nov 12, 2023

Hmm. Ok, but it seems the new formal way to access the header is through genl_info_userhdr(), not by manual pointer manipulation.

(Notes moved to the review)

@@ -14,7 +14,7 @@ char *get_iname(struct genl_info *info)

struct joolnlhdr *get_jool_hdr(struct genl_info *info)
{
return info->userhdr;
return (struct joolnlhdr *)(info->genlhdr + GENL_HDRLEN);
Copy link
Member

@ydahhrk ydahhrk Nov 12, 2023

Choose a reason for hiding this comment

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

struct genlmsghdr is a four byte structure (excluding unlikely padding and slop), which means if you add a number to a genlmsghdr pointer, you will actually sum four times the indicated amount.

In other words, you're returning the equivalent of

((u8 *)info->genlhdr) + 4 * GENL_HDRLEN

When (according to genl_info_userhdr()) you actually want to return

((u8 *)info->genlhdr) + GENL_HDRLEN

Try compiling the following userspace program to see the difference:

#include <stdio.h>
#include <linux/genetlink.h>

int main(int argc, char **argv)
{
	struct genlmsghdr base;
	struct genlmsghdr *ptr;

	ptr = &base;

	printf("ptr              : %p\n", ptr);
	/* Prints the location right next to base. */
	printf("ptr + 1          : %p\n", ptr + 1);
	/* Prints a location far to base's right */
	printf("ptr + GENL_HDRLEN: %p\n", ptr + GENL_HDRLEN);

	return 0;
}

(Dump that into a file called pointer-test.c, then run something like gcc pointer-test.c && ./a.out)

@ydahhrk
Copy link
Member

ydahhrk commented Nov 12, 2023

Note, we have helper macros to define code that should be compiled differently depending on linked kernel version. Sample usage.

@runborg
Copy link

runborg commented Nov 12, 2023

As a note, there is a patch from canonical/ubuntu for this: https://git.launchpad.net/ubuntu/+source/jool/tree/debian/patches/0002-Linux-6.6-support.patch?h=applied/ubuntu/devel

@ydahhrk
Copy link
Member

ydahhrk commented Nov 14, 2023

Just to be clear: I'm waiting for a patch because I think it's rude to overwrite people's contributions. (Though admittedly I've done it in the past.)

I'll wait until Saturday. Alternatively, if you just want me to push the fix ASAP, just say so.

Commit bffcc6882a "genetlink: remove userhdr from struct genl_info"
caused the build to fail since the field no longer exists.

Replace with run-time calculation of the header offset.

Signed-off-by: Tj <linux@iamtj>
@iam-TJ
Copy link
Author

iam-TJ commented Nov 16, 2023

I've been away for a few days; thanks for the review. Looking at it now I'm not quite sure what I was thinking but assume I copied from genl_info_userhdr() but replaced the u8 * cast rather than adding the additional cast.

Maybe the Canonical patch makes more sense since it makes clear that internal kernel structure changed at version 6.6.

@ydahhrk ydahhrk merged commit 741baa8 into NICMx:main Nov 17, 2023
@ydahhrk ydahhrk added this to the 4.1.11 milestone Dec 22, 2023
Copy link

@microcai microcai left a comment

Choose a reason for hiding this comment

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

I object this change. this will cause ABI breakage and not ever noticed by future compiler errors.

ydahhrk added a commit that referenced this pull request Aug 26, 2024
@ydahhrk
Copy link
Member

ydahhrk commented Aug 26, 2024

@microcai How about this?

@microcai
Copy link

@ydahhrk this one is nice !

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

Successfully merging this pull request may close these issues.

4 participants