On Thu, Jan 4, 2018 at 8:28 AM, Jason Wang [off-list ref] wrote:

On 2018年01月02日 17:19, Willem de Bruijn wrote:

quoted

quoted

quoted

More importantly, should this program just return a boolean pass or
drop. Taking a length and trimming may introduce bugs later on if the
stack parses the packet unconditionally, expecting a minimum size
to be present.

This was the reason for introducing sk_filter_trim_cap and using that
in other sk_filter sites.

A quick scan shows that tun_put_user expects a full vlan tag to exist
if skb_vlan_tag_present(skb), for instance. If trimmed to below this
length the final call to skb_copy_datagram_iter may have negative
length.

This is an issue with the existing sk_filter call as much as with the
new run_ebpf_filter call.

Good point, so consider it was used by sk_filter too, we need to fix it
anyway. Actually, I've considered the boolean return value but finally I
decide to obey the style of sk filter. Maybe the trimming has real user.
e.g
high speed header recoding/analysis? Consider it's not hard to fix, how
about just keep that?

I don't see an obvious use case, but sure. We'll just need to look
at what the minimum trim length needs to be.

Try to reproduce the possible issue, but looks like we are safe since we may
hit -EFAULT which is returned by skb_copy_datagram_iter() before.

Ah, indeed, it can handle short lengths. Great, thanks for checking.

So in V2,
I will keep the code as is except trim 4 more bytes if vlan tag is present.

This is do not follow. Perhaps the patch will make it clear.