linux-kernel - Re: [PATCH] staging: unisys: uislib: uisqueue.c: fixed sparse warning of context imbalance

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]

Date:	Tue, 2 Sep 2014 17:05:32 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
Cc:	Benjamin Romer <benjamin.romer@...sys.com>,
	David Kershner <david.kershner@...sys.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devel@...verdev.osuosl.org, sparmaintainer@...sys.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: unisys: uislib: uisqueue.c: fixed sparse
 warning of context imbalance

On Tue, Sep 02, 2014 at 04:39:47PM +0530, Sudip Mukherjee wrote:
> fixed sparse warning : context imbalance in 'do_locked_client_insert'
> 			 different lock contexts for basic block
> 
> spin_unlock_irqrestore is called at a later stage before returning 
> from the function if locked is 1.
> 
> Signed-off-by: Sudip Mukherjee <sudip@...torindia.org>

This doesn't match the email address you are using.

Really, your patch isn't bad but I would prefer if you re-wrote this
entire function because currently it is garbage.

static u8
do_locked_client_insert(struct uisqueue_info *queueinfo,
                        unsigned int whichqueue,
                        void *pSignal,
                        spinlock_t *lock,
                        unsigned char issueInterruptIfEmpty,
                        u64 interruptHandle, u8 *channelId)
{
        unsigned long flags;
        unsigned char queueWasEmpty;

        spin_lock_irqsave(lock, flags);

        if (!ULTRA_CHANNEL_CLIENT_ACQUIRE_OS(queueinfo->chan, channelId, NULL))
                goto unlock;

        queueWasEmpty = visor_signalqueue_empty(queueinfo->chan, whichqueue);
        if (!visor_signal_insert(queueinfo->chan, whichqueue, pSignal))
                goto release;
        ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId, NULL);
        spin_unlock_irqrestore(lock, flags);

        queueinfo->packets_sent++;

        return 1;

release:
        ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId, NULL);
unlock:
        spin_unlock_irqrestore(lock, flags);

        return 0;
}

The queueWasEmpty variable is kind of silly.  It should just be an int
or maybe a bool if you are being pedantic but instead we very
specifically set it to be an unsigned variable of the incorrect type.
Also we don't use queueWasEmpty at all.  I think we could delete it...

The problem with the original code was that the error paths and the
success paths were mixed together like spaghetti.  If you separate them
out and unwind in the proper order with normal label names then the
code is easy to understand.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/