netdev - Re: [linux-nfc] [PATCH 1/8] NFC: NCI: Allow connection close with dev down

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAFPB+YfYBMd0Ubtt-EhuDuyAiy2orPnPNkwV_hs=JTmeKW_GaQ@mail.gmail.com>
Date:	Fri, 28 Aug 2015 17:05:39 +0300
From:	Robert Dolca <robert.dolca@...il.com>
To:	Samuel Ortiz <sameo@...ux.intel.com>
Cc:	Robert Dolca <robert.dolca@...el.com>, linux-nfc@...ts.01.org,
	netdev@...r.kernel.org, linux-wireless@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [linux-nfc] [PATCH 1/8] NFC: NCI: Allow connection close with dev down

On Sun, May 24, 2015 at 8:07 PM, Samuel Ortiz <sameo@...ux.intel.com> wrote:
> Hi Robert,
>
> On Tue, Mar 31, 2015 at 05:03:42PM +0300, Robert Dolca wrote:
>> On Thu, Mar 26, 2015 at 2:29 AM, Samuel Ortiz <sameo@...ux.intel.com> wrote:
>> > Hi Robert,
>> >
>> > On Tue, Feb 24, 2015 at 12:01:45PM +0200, Robert Dolca wrote:
>> >> By calling __nci_request instead of nci_request allows the driver to use
>> >> the function while initializing the device (setup stage)
>> >>
>> >> Signed-off-by: Robert Dolca <robert.dolca@...el.com>
>> >> ---
>> >>  net/nfc/nci/core.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
>> >> index 9575a18..c4dd5d8 100644
>> >> --- a/net/nfc/nci/core.c
>> >> +++ b/net/nfc/nci/core.c
>> >> @@ -558,7 +558,7 @@ static void nci_core_conn_close_req(struct nci_dev *ndev, unsigned long opt)
>> >>
>> >>  int nci_core_conn_close(struct nci_dev *ndev, u8 conn_id)
>> >>  {
>> >> -     return nci_request(ndev, nci_core_conn_close_req, conn_id,
>> >> +     return __nci_request(ndev, nci_core_conn_close_req, conn_id,
>> >>                               msecs_to_jiffies(NCI_CMD_TIMEOUT));
>> > You're fixing your problem by removing the NCI request serialization and
>> > removing the check for your device being UP.
>> > I assume you need to open and close a proprietary connection from your
>> > setup hook ? Then please extend nci_request() to check for both NCI_UP
>> > and NCI_INIT.
>>
>> You are right, I am opening and closing a connection from the setup
>> function. The setup is called by nci_open_device. At the beginning of
>> nci_open_device, req_lock is being acquired and it is release at the
>> end of the function. That means that when setup is being called
>> req_lock is acuired. As you said I can modify nci_request to check for
>> NCI_INIT but it tries to acquire req_lock and it can not succeed.
> I see, I thought the issue was only about checking the NCI_* flags.
>
> As a short term solution, I propose you do the following:
>
> a) Export nci_core_conn_create_req, nci_core_conn_close_req and
> __nci_request.
> b) Call __nci_request() directly from your fdp_nci_close_conn() and
> fdp_nci_create_conn() routines.
>
> The long term, scalable fix would be to implement and export an
> __nci_send_cmd_sync() routine, that would transparently build an NCI
> request and tail it to the ndev req skb queue, and put the caller on a
> wait queue. The created request's response callback would then wake the
> caller up.

If nci_open_device would use another mutex instead of req_lock this
wouldn't be necessary.
I don't see any reason why nci_open_device should block the send
queue. Of course, in nci_open_device all calls to __nci_request would
have to be replaced with nci_request.

Samuel, would that be an acceptable solution?

Regards,
Robert
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html