Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210121002129.93754-1-xie.he.0141@gmail.com> <b42575d44fb7f5c1253635a19c3e21e2@dev.tdt.de>
 <20210123204507.35c895db@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
In-Reply-To: <20210123204507.35c895db@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
From:   Xie He <xie.he.0141@gmail.com>
Date:   Sun, 24 Jan 2021 03:19:35 -0800
Message-ID: <CAJht_ENQ2aN2vvWzLDGUUk-7Yv_=UUJmOrbC3M9J=j5uET-pAQ@mail.gmail.com>
Subject: Re: [PATCH net v5] net: lapb: Add locking to the lapb module
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     Martin Schiller <ms@dev.tdt.de>,
        "David S. Miller" <davem@davemloft.net>,
        Linux X25 <linux-x25@vger.kernel.org>,
        Linux Kernel Network Developers <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Jan 23, 2021 at 8:45 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> > > @@ -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.

> > > -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".