- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.2k
adding ethhdr type for linux/android for proper packet filtering. #4239
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
base: main
Are you sure you want to change the base?
Conversation
| Some changes occurred in the Android module cc @maurer | 
55f537d    to
    d478bbb      
    Compare
  
    da428da    to
    4f428e0      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main thing I see missing here is a use of ethhdr. A struct type with no usage by libc might not actually belong here, even though it is on the UAPI interface - this is a bindings crate for libc, not for the Linux kernel.
Is there a function in libc that you intend to call that uses the ethhdr struct? If so, layering on a commit binding that on top of this one would make a better PR IMO.
        
          
                src/unix/linux_like/android/mod.rs
              
                Outdated
          
        
      | } | ||
|  | ||
| // is __be16 __bitwise __u16 | ||
| pub struct __c_anonymous___be16 { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why __c_anonymous_? This isn't an anonymous type, it's name is __be16.
Unless I'm mistaken, __c_anonymous_fieldname is normally used for inline structs/unions attached to a field of that name - this is a named type, and that's not the field name.
        
          
                src/unix/linux_like/android/mod.rs
              
                Outdated
          
        
      |  | ||
| impl Eq for ethhdr {} | ||
|  | ||
| impl PartialEq for ethhdr { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PartialEq definition appears to lack a check on the h_proto field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not forget but until I solve the alignment issue in some platforms and as I was playing with the h_proto type I do not bother with it just yet (still a draft).
        
          
                src/unix/linux_like/android/mod.rs
              
                Outdated
          
        
      | self.h_dest | ||
| .iter() | ||
| .zip(other.h_dest.iter()) | ||
| .all(|(a, b)| a == b) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you not want them to compare equal if they have the same CStr value, but different null trailing bytes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that those fields represent mac addresses not as strings (note they re unsigned char).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my bad, you're correct.
        
          
                src/unix/linux_like/android/mod.rs
              
                Outdated
          
        
      | } | ||
|  | ||
| // is __be16 __bitwise __u16 | ||
| pub struct __c_anonymous___be16 { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider deriving Eq/PartialEq, adding a method to access/set it as host u16, and adding a Debug based on the host representation?
If you do that, your later stuff on ethhdr can just be a derive.
        
          
                src/unix/linux_like/android/mod.rs
              
                Outdated
          
        
      |  | ||
| // linux/if_ether.h | ||
|  | ||
| #[repr(C, align(1))] | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This struct is packed and not align-1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. was trying to solve few architectures build failures.
        
          
                src/unix/linux_like/android/mod.rs
              
                Outdated
          
        
      | f.debug_struct("ethhdr") | ||
| .field("h_dest", &self.h_dest) | ||
| .field("h_source", &self.h_source) | ||
| .finish() | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing proto field
        
          
                src/unix/linux_like/linux/mod.rs
              
                Outdated
          
        
      | } | ||
|  | ||
| // is __be16 __bitwise __u16 | ||
| pub struct __c_anonymous___be16 { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above re: __c_anonymous - it also might be helpful just to have a type like this higher up in the hierarchy to be shared, I find the idea that only Linux/Android have bigendian u16s unlikely.
| 
 There is, for packet filters purpose. sendto/recvfrom can use any buffer e.g. ethhdr for this case. since AF_PACKET family and ETH_P* constants are already there, I m just trying to finish it off. | 
9bb6cb5    to
    a8ed135      
    Compare
  
    930a40c    to
    9944b71      
    Compare
  
    | Since this is marked as a draft: @rustbot author (just update the labels if that isn't accurate) | 
3c77dba    to
    7357526      
    Compare
  
    | This will need a rebase but I think it's ready for review so updating the labels @rustbot review | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this fell off my radar. Just a few requests/questions
| // FIXME: `h_proto` is of type __be16 big endian version of __u16 | ||
| (struct_ == "ethhdr" && field == "h_proto") || | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What doesn't it like here? As far as I can tell, the __be16 type just gets the bitwise attribute which isn't picked up by GCC https://github.com/torvalds/linux/blob/e53642b87a4f4b03a8d7e5f8507fc3cd0c595ea6/include/uapi/linux/types.h#L27-L41.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devnexen what are the errors with this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i forgot since, gonna try removing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall just now it was for some archs only.
1fa2062    to
    f92ae26      
    Compare
  
    | This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. | 
f69471d    to
    0019422      
    Compare
  
    | @rustbot ready | 
6fc575c    to
    94f06b3      
    Compare
  
    
No description provided.