Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 26 Jan 2018 11:07:39 -0500 (EST)
Message-Id: <20180126.110739.419621238821097728.davem@davemloft.net>
To:     viro@ZenIV.linux.org.uk
Cc:     baijiaju1990@gmail.com, 3chas3@gmail.com,
        linux-atm-general@lists.sourceforge.net, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] atm: firestream: Replace GFP_ATOMIC with GFP_KERNEL in
 fs_send
From:   David Miller <davem@davemloft.net>
In-Reply-To: <20180126120522.GX13338@ZenIV.linux.org.uk>
References: <1516953627-30983-1-git-send-email-baijiaju1990@gmail.com>
        <20180126120522.GX13338@ZenIV.linux.org.uk>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

From: Al Viro <viro@ZenIV.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.