Fix parsing of non-dhcp packets

[agorgl]

Fixes #32

[agorgl]

@cathay4t Can you take a look at this?

[cathay4t]

@agorgl We have BPF filter enabled, why we will receive non-DHCP package?

[cathay4t]

On second through, yes, mal-formated ethernet package will trigger exception in our code.

I am OK to ignore invalid DHCP packet. Please include a log::trace when dropping a packet.

[agorgl]

On second through, yes, mal-formated ethernet package will trigger exception in our code.

I am OK to ignore invalid DHCP packet. Please include a log::trace when dropping a packet.

Added a trace log as requested, are you sure you don't want this to be a debug log?

[agorgl]

@cathay4t Are we good now?

[cathay4t]

Trace log is off by default in many log provider, so it will not impact any user but good for debugging.

• The from_dhcp_pkg() already return Result<>, please move is_dhcp_pkg() into from_dhcp_pkg() by raising special kind of error, which then caller can determine to ignore or not.
• Looping here will cause blocking if there is no pkg coming afterwards. Currently we have retry method, no need to retry here.
• Please explain why IPv6 multicast packet passed the BPF filter. And the fix should be in BPF filter bpf.rs.
• Even somehow we got a malformed DHCP packet, existing mozim code should just drop it and waiting next packet to arrive. No use case should be impacted, please investigate why it breaks your use case.

So, no, this PR is not ready to merge.

[cathay4t]

We have

    // Move on if ETHERTYPE_IP, otherwise drop package
    (BPF_JMP | BPF_JEQ | BPF_K, 0, 8, ETHERTYPE_IP),

In BPF filter, so it is hard for me to understand why IPv6 multicast package passed this BPF filter.

[agorgl]

We have

    // Move on if ETHERTYPE_IP, otherwise drop package
    (BPF_JMP | BPF_JEQ | BPF_K, 0, 8, ETHERTYPE_IP),

In BPF filter, so it is hard for me to understand why IPv6 multicast package passed this BPF filter.

I don't know either why it passed, but the behavior is trivially reproducible in the steps described in the podman/netavark poc

[cathay4t]

Can you provide a reproducer for me to debug?

[agorgl]

Yes, the repro case is here: https://github.com/containers/netavark/issues/618

[agorgl]

• The from_dhcp_pkg() already return Result<>, please move is_dhcp_pkg() into from_dhcp_pkg() by raising special kind of error, which then caller can determine to ignore or not.

The is_dhcp_pkg() processes a raw packet buffer, do you mean to move this into from_eth_pkg()?

• Looping here will cause blocking if there is no pkg coming afterwards. Currently we have retry method, no need to retry here.

Can you pinpoint where the retry happens?

• Even somehow we got a malformed DHCP packet, existing mozim code should just drop it and waiting next packet to arrive. No use case should be impacted, please investigate why it breaks your use case.

The poc provided above proves it doesn't for some reason

[agorgl]

Moved dhcp packet validation logic inside from_eth_pkg() and we now return InvalidDhcpServerReply on invalid DHCP packets

[agorgl]

@cathay4t How should we go forward on this?

[cathay4t]

Aha, I have only retry in process_renew_recv(). Let me create a PR for this. looping is not acceptable.

[cathay4t]

https://github.com/nispor/mozim/pull/34 created for this. Could you try it in your env?

[agorgl]

Yes, let me check it

[agorgl]

Deprecated in favor of #34 and #35