lkml.org 
[lkml]   [2018]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] atm: firestream: Replace GFP_ATOMIC with GFP_KERNEL in fs_send
    From
    Date


    On 2018/1/27 1:08, Al Viro wrote:
    > On Fri, Jan 26, 2018 at 11:07:39AM -0500, David Miller wrote:
    >>>> 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.
    > FWIW, the tool *does* promise to be useful

    Thanks, I am happy to hear that :)

    > - as far as I understand it
    > looks for places where GFP_ATOMIC allocation goes with blocking operations
    > near every callchain leading there. And that indicates something fishy
    > going on - either pointless GFP_ATOMIC (in benign case), or something
    > much nastier: a callchain that would require GFP_ATOMIC. In that case
    > whatever blocking operation found along the way is a bug.

    In fact, my tool first collects all places of GFP_ATOMIC and mdelay in
    the whole kernel code.
    Then it starts analysis from the entry of each interrupt handler and
    spin-lock function call in the whole kernel code,
    to mark the places of GFP_ATOMIC and mdelay that are called in atomic
    context.
    The remaining unmarked places of GFP_ATOMIC and mdelay are reported and
    can be replaced with GFP_KERNEL and mdelay (or usleep_range).

    Though my tool has handled some common situations of function pointers,
    But it does not well handle function pointer passing in this code
    (vcc->send = vcc->dev->ops->send), so the tool needs to be improved.
    I am sorry for my incorrect report...

    > This time it has, AFAICS, caught a genuine bug in drivers/atm/firestream.c -
    > static void submit_qentry (struct fs_dev *dev, struct queue *q, struct FS_QENTRY *qe)
    > {
    > u32 wp;
    > struct FS_QENTRY *cqe;
    >
    > /* XXX Sanity check: the write pointer can be checked to be
    > still the same as the value passed as qe... -- REW */
    > /* udelay (5); */
    > while ((wp = read_fs (dev, Q_WP (q->offset))) & Q_FULL) {
    > fs_dprintk (FS_DEBUG_TXQ, "Found queue at %x full. Waiting.\n",
    > q->offset);
    > schedule ();
    > }
    > ...
    > static void submit_queue (struct fs_dev *dev, struct queue *q,
    > u32 cmd, u32 p1, u32 p2, u32 p3)
    > {
    > struct FS_QENTRY *qe;
    >
    > qe = get_qentry (dev, q);
    > qe->cmd = cmd;
    > qe->p0 = p1;
    > qe->p1 = p2;
    > qe->p2 = p3;
    > submit_qentry (dev, q, qe);
    > ...
    > static int fs_send (struct atm_vcc *atm_vcc, struct sk_buff *skb)
    > {
    > ...
    > td = kmalloc (sizeof (struct FS_BPENTRY), GFP_ATOMIC);
    > ...
    > submit_queue (dev, &dev->hp_txq,
    > QE_TRANSMIT_DE | vcc->channo,
    > virt_to_bus (td), 0,
    > virt_to_bus (td));
    > ...
    >
    > Either all callchains leading to fs_send() are in non-atomic contexts
    > (in which case that GFP_ATOMIC would be pointless) or there's one
    > where we cannot block. Any such would be a potential deadlock.
    >
    > And the latter appears to be the case - fs_send() is atmdev_ops ->send(),
    > which can end up in atm_vcc ->send(), which can be called from under
    > ->sk_lock.slock.

    Yes, I think schedule() can cause a sleep-in-atomic-context bug.

    > What would be really useful:
    > * list of "here's a list of locations where we do something
    > blocking; each callchain to this GFP_ATOMIC allocation passes either
    > upstream of one of those without leaving atomic context in between
    > or downstream without entering one".
    > * after that - backtracking these callchains further, watching
    > for ones in atomic contexts. Can be done manually, but if that tool
    > can assist in doing so, all the better. If we do find one, we have
    > found a deadlock - just take the blocking operation reported for
    > that callchain and that's it. If it's not an obvious false positive
    > (e.g.
    > if (!foo)
    > spin_lock(&splat);
    > .....
    > if (foo)
    > schedule();
    > ), it's worth reporting to maintainers of the code in question.
    > * if all callchains reach obviously non-atomic contexts
    > (syscall entry, for example, or a kernel thread payload, or
    > a function documented to require a mutex held by caller, etc.) -
    > then a switch to GFP_KERNEL might be appropriate. With analysis
    > of callchains posted as you are posting that.
    > * either way, having the tool print the callchains out
    > would be a good idea - for examining them, for writing reports,
    > etc.

    Thanks for your very helpful advice :)
    I will follow it in my patches.


    Thanks,
    Jia-Ju Bai

    \
     
     \ /
      Last update: 2018-01-27 05:10    [W:8.699 / U:0.124 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site