Re: [PATCH net-next 3/3] strparser: Generalize strparser

On 10/19/2017 12:51 PM, Tom Herbert wrote:

On Thu, Oct 19, 2017 at 10:42 AM, Eric Dumazet [off-list ref]
wrote:

quoted

On Fri, 2017-07-28 at 16:22 -0700, Tom Herbert wrote:

quoted

Generalize strparser from more than just being used in conjunction
with read_sock. strparser will also be used in the send path with
zero proxy. The primary change is to create strp_process function
that performs the critical processing on skbs. The documentation
is also updated to reflect the new uses.

Signed-off-by: Tom Herbert <redacted>
---
 Documentation/networking/strparser.txt | 207 +++++++++++++++-------
 include/net/strparser.h                | 119 +++++++------
 net/kcm/kcmproc.c                      |  34 ++--
 net/kcm/kcmsock.c                      |  38 ++--
 net/strparser/strparser.c              | 313

++++++++++++++++++++-------------

quoted

 5 files changed, 424 insertions(+), 287 deletions(-)

Just found this gem :

static void strp_msg_timeout(unsigned long arg)
{
        struct strparser *strp = (struct strparser *)arg;

        /* Message assembly timed out */
        STRP_STATS_INCR(strp->stats.msg_timeouts);
        strp->cb.lock(strp);
        strp->cb.abort_parser(strp, ETIMEDOUT);
        strp->cb.unlock(strp);
}

static void strp_sock_lock(struct strparser *strp)
{
        lock_sock(strp->sk);
}

static void strp_sock_unlock(struct strparser *strp)
{
        release_sock(strp->sk);
}


A timer runs from BH, and from this interrupt context it is absolutely
illegal to call lock_sock() ( and release_sock() )

Please fix, thanks !

Nice catch! I'll fix it.

Can the lock/release be removed and the strp stopped bit can use atomics?

It looks like sk_error_report can be called without sock lock.

Thanks,
John