netdev - Re: [Patch net-next v3 0/3] net_sched: reflect tx_queue_len change for pfifo

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]

Date:   Sun, 28 Jan 2018 22:09:47 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [Patch net-next v3 0/3] net_sched: reflect tx_queue_len change
 for pfifo_fast

On 01/28/2018 09:57 PM, Cong Wang wrote:
> On Sun, Jan 28, 2018 at 9:35 PM, John Fastabend
> <john.fastabend@...il.com> wrote:
>> On 01/25/2018 06:26 PM, Cong Wang wrote:
>>> This pathcset restores the pfifo_fast qdisc behavior of dropping
>>> packets based on latest dev->tx_queue_len. Patch 1 introduces
>>> a helper, patch 2 introduces a new Qdisc ops which is called when
>>> we modify tx_queue_len, patch 3 implements this ops for pfifo_fast.
>>>
>>> Please see each patch for details.
>>>
>>
>> Overall this series is better than what we have at the moment, but
>> a better fix would preallocate the memory, to avoid ENOMEM errors,
> 
> I am not against for better ENOMEM error handling, but I still have to
> remind you that attach_one_default_qdisc() doesn't handle it either.
> Since no one complained about it, why this one is so special?

Its not we should fix both cases. And also clean up the multiple
net sync calls in these paths as well.

> 
> 
>> and add a ptr_ring API to use the preallocated buffers.
> 
> 
> What ptr_ring API could cure netdev tx queues problem here?
> Looks like you still don't understand the problem here.
> 

The resize multiple array API can only fail due to alloc failures.
We need to break this API into two pieces preallocate the memory
and then commit array changes. Alternatively the qdisc layer could
allocate new arrays and then swap them after all the arrays been
initialized correctly using ptr_ring APIs below the ptr_ring
resize API calls.

Having ptr_ring API operations to support this seems best.

> 
>>
>> We have time (its only in net-next) so lets do the complete fix
>> rather than this series IMO.
>>
> 
> Why not just accept this and complete the error handling later
> given the fact that I already add a TODO? IOW, why it is now?
> 

Because its not correct and its not too much work to get it so
the error is avoided.

I don't mind if the patches go in as-is a follow up patch can
also fix the TODO item.

.John