netdev - Re: [Patch net] atm: remove an unnecessary loop

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: <20170113132346.GA19220@electric-eye.fr.zoreil.com>
Date:   Fri, 13 Jan 2017 14:23:46 +0100
From:   Francois Romieu <romieu@...zoreil.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     netdev@...r.kernel.org, mhocko@...nel.org, 3chas3@...il.com,
        andreyknvl@...gle.com
Subject: Re: [Patch net] atm: remove an unnecessary loop

Cong Wang <xiyou.wangcong@...il.com> :
[...]
> alloc_skb(GFP_KERNEL) itself is sleeping, so the new wait api is still
> needed.

The task state change warning is the symptom.

The deeply nested alloc_skb is the problem.

Diagnosis: nesting is wrong. It makes zero sense. Fix it and the
implicit task state change problem automagically goes away.

alloc_skb() does not need to be in the "while" loop.

alloc_skb() does not need to be in the {prepare_to_wait/add_wait_queue ...
finish_wait/remove_wait_queue} block.

alloc_tx() is not correctly named: given its original content, it deserves
to be called something like:

"wait_for_decent_tx_drain_and_alloc_by_hand_coz_i_dont_trust_the_mm_subsystem_and_i_dont_know_what_i_want"

I claim that:
- alloc_tx() should only perform the "wait_for_decent_tx_drain" part
- alloc_skb() ought to be done directly in vcc_sendmsg
- alloc_skb() failure can be handled gracefully in vcc_sendmsg
- alloc_skb() may use a (m->msg_flags & MSG_DONTWAIT) dependant
  GFP_{KERNEL / ATOMIC} flag
- most of it can be done in a longterm maintenance pain minimizing
  way. Call it a side-effect: I don't claim that it *must* be done
  this way.

-- 
Ueimor