linux-kernel - Re: [PATCH] atm: firestream: Replace GFP_ATOMIC with GFP_KERNEL in fs

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: <20180126.110739.419621238821097728.davem@davemloft.net>
Date:   Fri, 26 Jan 2018 11:07:39 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     viro@...IV.linux.org.uk
Cc:     baijiaju1990@...il.com, 3chas3@...il.com,
        linux-atm-general@...ts.sourceforge.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] atm: firestream: Replace GFP_ATOMIC with GFP_KERNEL in
 fs_send

From: Al Viro <viro@...IV.linux.org.uk>
Date: Fri, 26 Jan 2018 12:05:22 +0000

> On Fri, Jan 26, 2018 at 04:00:27PM +0800, Jia-Ju Bai wrote:
>> After checking all possible call chains to fs_send() here,
>> my tool finds that fs_send() is never called in atomic context.
>> And this function is assigned to a function pointer "dev->ops->send",
>> which is only called by vcc_sendmsg() (net/atm/common.c)
>> through vcc->dev->ops->send(), and vcc_sendmsg() calls schedule(),
>> it indicates that fs_send() can call functions which may sleep.
>> Thus GFP_ATOMIC is not necessary, and it can be replaced with GFP_KERNEL.
>> 
>> This is found by a static analysis tool named DCNS written by myself.
> 
> The trouble is, places like
> net/atm/raw.c:65:       vcc->send = atm_send_aal0;
> net/atm/raw.c:74:       vcc->send = vcc->dev->ops->send;
> net/atm/raw.c:83:       vcc->send = vcc->dev->ops->send;
> mean extra call chains.  It's *not* just vcc_sendmsg(), and e.g.
>         ret = ATM_SKB(skb)->vcc->send(ATM_SKB(skb)->vcc, skb)
>             ? DROP_PACKET : 1;
>         bh_unlock_sock(sk_atm(vcc));
> in pppoatm_send() definitely is called under a spinlock.
> 
> Looking through the driver (in advanced bitrot, as usual for drivers/atm),
> I'd say that submit_queue() is fucked in head in the "queue full" case.
> And judging by the history, had been thus since the original merge...

Jia-Ju, I'm probably not going to apply any of your GFP_KERNEL
conversions.

Al's analysis above is similar to how things looked for other patches
you submited of this nature.

So because of the lack of care and serious auditing you put into these
conversions, I really have no choice but to drop them from my queue
because on the whole they are adding bugs rather than improving the
code.

Thank you.