Re: [PATCH net v5] net: lapb: Add locking to the lapb module

On Sat, Jan 23, 2021 at 8:45 PM Jakub Kicinski [off-list ref] wrote:

quoted

quoted

@@ -178,11 +182,23 @@ int lapb_unregister(struct net_device *dev)
            goto out;
    lapb_put(lapb);

+   /* Wait for other refs to "lapb" to drop */
+   while (refcount_read(&lapb->refcnt) > 2)
+           ;

Tight loop like this is a little scary, perhaps add a small
usleep_range() here?

OK, sure. I'll add a usleep_range(1, 10) here.

quoted

quoted

-int lapb_disconnect_request(struct net_device *dev)
+static int __lapb_disconnect_request(struct lapb_cb *lapb)
 {
-   struct lapb_cb *lapb = lapb_devtostruct(dev);
-   int rc = LAPB_BADTOKEN;
-
-   if (!lapb)
-           goto out;
-
    switch (lapb->state) {
    case LAPB_STATE_0:
-           rc = LAPB_NOTCONNECTED;
-           goto out_put;
+           return LAPB_NOTCONNECTED;

    case LAPB_STATE_1:
            lapb_dbg(1, "(%p) S1 TX DISC(1)\n", lapb->dev);

@@ -310,12 +328,10 @@ int lapb_disconnect_request(struct net_device

*dev)
            lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON, LAPB_COMMAND);
            lapb->state = LAPB_STATE_0;
            lapb_start_t1timer(lapb);
-           rc = LAPB_NOTCONNECTED;
-           goto out_put;
+           return LAPB_NOTCONNECTED;

    case LAPB_STATE_2:
-           rc = LAPB_OK;
-           goto out_put;
+           return LAPB_OK;
    }

    lapb_clear_queues(lapb);

@@ -328,8 +344,22 @@ int lapb_disconnect_request(struct net_device

*dev)
    lapb_dbg(1, "(%p) S3 DISC(1)\n", lapb->dev);
    lapb_dbg(0, "(%p) S3 -> S2\n", lapb->dev);

-   rc = LAPB_OK;
-out_put:
+   return LAPB_OK;
+}

Since this is a fix for net, I'd advise against converting the goto
into direct returns (as much as I generally like such conversion).

This part is actually splitting "lapb_disconnect_request" into two
functions - a "__lapb_disconnect_request" without locking, and a
"lapb_disconnect_request" which provides the locking and calls
"__lapb_disconnect_request". The splitting is necessary for
"lapb_device_event" to directly call "__lapb_disconnect_request" with
the lock already held. After the splitting, the "out_put" tag would
actually be in the caller function so there's nowhere we can "goto".