netdev - Re: [PATCH ipsec] xfrmi: drop ignore

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5F295578.4040004@mail.wizbit.be>
Date:   Tue, 04 Aug 2020 14:32:56 +0200
From:   Bram Yvakh <bram-yvahk@...l.wizbit.be>
To:     sd@...asysnail.net
CC:     netdev@...r.kernel.org,
        Steffen Klassert <steffen.klassert@...unet.com>, xmu@...hat.com
Subject: Re: [PATCH ipsec] xfrmi: drop ignore_df check before updating pmtu

On 4/08/2020 11:37, Sabrina Dubroca wrote:
> xfrm interfaces currently test for !skb->ignore_df when deciding
> whether to update the pmtu on the skb's dst. Because of this, no pmtu
> exception is created when we do something like:
>
>     ping -s 1438 <dest>
>
> By dropping this check, the pmtu exception will be created and the
> next ping attempt will work.
>
>
> Fixes: f203b76d7809 ("xfrm: Add virtual xfrm interfaces")
> Reported-by: Xiumei Mu <xmu@...hat.com>
> Signed-off-by: Sabrina Dubroca <sd@...asysnail.net>
> ---
>  net/xfrm/xfrm_interface.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
> index b615729812e5..ade2eba863b3 100644
> --- a/net/xfrm/xfrm_interface.c
> +++ b/net/xfrm/xfrm_interface.c
> @@ -292,7 +292,7 @@ xfrmi_xmit2(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
>  	}
>  
>  	mtu = dst_mtu(dst);
> -	if (!skb->ignore_df && skb->len > mtu) {
> +	if (skb->len > mtu) {
>   

To me dropping the 'ignore_df' check looks incorrect;
When I submitted patches last year for VTI which related to
ptmu/df-bit[1]  I did some testing and also some comparison (also with GRE)
(I also briefly tested with xfrmi but given that the VTI patches were
mostly ignored I did not pursue that further[2])

The conclusion for my testing with GRE: (only the last item is relevant
but rest included for context)
* with 'pmtudisc' set for the GRE device: outgoing GRE packet has the
DF-bit set (this did suffer from some issues when the
to-be-forwarded-packet did not have the DF bit set)
* with 'nopmtudisc' set for the GRE device: outgoing GRE packet copies
DF-bit from the to-be-forwarded packet (this did suffer from some issues
when client did set DF bit..)
* with 'nopmtudisc' and 'ignore-df' bit set: outgoing GRE packet never
has the DF bit set: packet can be fragmented at will

How I understand things (based on my testing last year): when
'ignore-df' is set then the DF-bit should *not* be set. This in turns
means that path-mtu discovery is not possible (which is why it has to be
used in combination with 'nopmtudisc').
In the patch above the 'ignore_df' is removed which looks wrong: if
'ignore_df' is set then the size of the packet should not be checked
since the packet may be fragmented at will.
(I also suppose that means that setting 'ignore_df' is not an option (or
no longer) an option for xfrmi?)

I'm also not sure what the exact case is/was that lead to this patch;
can you shed some light on it?
(What I would expect: if 'ignore_df' is set then pmtu discovery does not
happen.)

[1]:
https://lore.kernel.org/netdev/1552865877-13401-1-git-send-email-bram-yvahk@mail.wizbit.be/
and
https://lore.kernel.org/netdev/1552865877-13401-2-git-send-email-bram-yvahk@mail.wizbit.be/
[2]: https://lore.kernel.org/netdev/5C8EDF7F.2010100@mail.wizbit.be/

>   
>  		skb_dst_update_pmtu_no_confirm(skb, mtu);
>  
>  		if (skb->protocol == htons(ETH_P_IPV6)) {
>