netdev - Re: [PATCH] USBNET: fix handling padding packet

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: <CACVXFVNp5Qj-EV+L=FezwFqRnFa7uRFca7Ju9=iTqU+DS1r-fg@mail.gmail.com>
Date:	Thu, 19 Sep 2013 01:03:31 +0800
From:	Ming Lei <ming.lei@...onical.com>
To:	Bjørn Mork <bjorn@...k.no>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Oliver Neukum <oneukum@...e.de>,
	Network Development <netdev@...r.kernel.org>,
	linux-usb <linux-usb@...r.kernel.org>,
	Oliver Neukum <oliver@...kum.org>
Subject: Re: [PATCH] USBNET: fix handling padding packet

On Wed, Sep 18, 2013 at 11:52 PM, Bjørn Mork <bjorn@...k.no> wrote:
> Ming Lei <ming.lei@...onical.com> writes:
>
>>> Are you really sure that the one driver/device using this really need
>>> the padding byte?  If you could just make FLAG_SEND_ZLP part of the
>>> condition for enabling can_dma_sg, then all this extra complexity would
>>> be unnecessary.  As the comment in front of the padding code says:
>>
>> At least for the effected driver of ax88179, the padding packet is needed
>> since the driver does set the padding flag in the header, see
>> ax88179_tx_fixup().
>
> Yes, I noticed that the driver doesn't set the flag. I just didn't put
> too much into that.  I don't think that necessarily means that the
> padding is needed. It probably just means that the driver worked with
> the default padding enabled, so the author left it there.
>
> This flag should really have been inverted.  ZLP should be the default
> for all new usbnet drivers.

In theory, it is, but who knows the reality.

>
> Why don't you test it on the device you tested the SG patch with?  I am
> pretty sure it works just fine using proper ZLP transfer termination.

I should have planned to test it, but didn't know how to build TCP TSO
to make the whole frame length plus 8 dividable by 1024.

Could you or other guys share how to build such packet so that I can
do the test?

Once we can confirm the padding isn't needed, we can remove the
padding flag. But if the padding flag is still there, this patch should be
dropped.

>
>>>   "strictly conforming cdc-ether devices should expect the ZLP here"
>>>
>>> There shouldn't be any problems requiring this conformance as a
>>> precondition for allowing SG.  The requirements are already strict.
>>
>> There is no reason to forbid DMA SG for one driver which requires
>> padding, right?
>
> Yes there is: Added complexity for everybody, based on a combination of
> features which just does not make any sense.
>
> No modern device should need the padding.  No old device will be able to
> use the SG feature as implemented. You only enable it on USB3, don't
> you? If this feature is restricted to USB3 capable devices, then it most
> certainly can be restricted to ZLP capable devices with absolutely no
> difference in the resulting set of supported devices.

See above, if we can prove that padding isn't needed, we can remove
the padding, then the patch can be dropped. If we can't, the patch is needed.

>
> Anyway, if you want to keep the padding for SG then maybe this will work
> and allow you to drop the extra struct usbnet field and allocation:
>
>                         if (skb_tailroom(skb) && !dev->can_dma_sg) {
>                                skb->data[skb->len] = 0;
>                                __skb_put(skb, 1);
>                         } else if (dev->can_dma_sg) {
>                               sg_set_buf(&urb->sg[urb->num_sgs++], skb->data, 1);
>                         }
>
> I.e. cheat and use the skb->data buffer twice, if that is allowed?  The
> actual value of the padding byte should not matter, I believe?

If so, we can remove the assignment of zero to skb->data[skb->len],
but probably it might cause regression, that is why I wouldn't like to
do that.  Also looks introducing one extra dev->padding_frame doesn't
cause much complexity.

Thanks
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html