Hi Robin,
On Tue, Jan 08, 2019 at 01:47:33PM +0100, Robin van der Gracht wrote:
> Hi Oleksij,
>
> On Tue, 8 Jan 2019 11:57:35 +0100
> Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
>
> > Hi Robin,
> >
> > On 08.01.19 11:26, Robin van der Gracht wrote:
> > > On Thu, 13 Dec 2018 16:31:27 +0100
> > > Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
> > >
> > >> Currently, (E)TP transfers will be aborted, as soon application will
> > >> call close() or exit, as the socket will be automatically closed by the
> > >> kernel.
> > >>
> > >> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > >> ---
> > >> net/can/j1939/socket.c | 10 ++++++++++
> > >> 1 file changed, 10 insertions(+)
> > >>
> > >> diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
> > >> index d6332483350f..e003d3cae24c 100644
> > >> --- a/net/can/j1939/socket.c
> > >> +++ b/net/can/j1939/socket.c
> > >> @@ -101,6 +101,13 @@ static inline void j1939_sock_pending_add(struct sock *sk)
> > >> atomic_inc(&jsk->skb_pending);
> > >> }
> > >>
> > >> +static int j1939_sock_pending_get(struct sock *sk)
> > >> +{
> > >> + struct j1939_sock *jsk = j1939_sk(sk);
> > >> +
> > >> + return atomic_read(&jsk->skb_pending);
> > >> +}
> > >> +
> > >> void j1939_sock_pending_del(struct sock *sk)
> > >> {
> > >> struct j1939_sock *jsk = j1939_sk(sk);
> > >> @@ -404,6 +411,9 @@ static int j1939_sk_release(struct socket *sock)
> > >> if (jsk->state & J1939_SOCK_BOUND) {
> > >> struct net_device *ndev;
> > >>
> > >> + wait_event_interruptible(jsk->waitq,
> > >> + j1939_sock_pending_get(&jsk->sk) == 0);
> > >> +
> > >> spin_lock_bh(&j1939_socks_lock);
> > >> list_del_init(&jsk->list);
> > >> spin_unlock_bh(&j1939_socks_lock);
> > >
> > > close() now blocks if data was successfully send on that socket.
> > > This is probably due to a resource issue which is now brought to light.
> > >
> > > Seems as if jsk->skb_pending is incremented on every j1939_sk_sendmsg()
> > > call, but only decremented when an error occurs. So when a send()
> > > succeeds, close() will block indefinitely.
> >
> >
> > It is decremented in:
> > static void __j1939_session_drop(struct j1939_session *session)
> > {
> > struct j1939_priv *priv = session->priv;
> > - struct sk_buff *se_skb = j1939_session_skb_find(session);
> >
> > - if (session->transmission) {
> > - if (se_skb && se_skb->sk)
> > - j1939_sock_pending_del(se_skb->sk);
> > - wake_up_all(&priv->tp_wait);
> > - }
> > + if (!session->transmission)
> > + return;
> > +
> > + j1939_sock_pending_del(session->sk);
> > + wake_up_all(&priv->tp_wait);
> > }
> >
> > Is it a theoretical or real issue?
>
> Real issue. Forgot to mention that I'm not using a transport session.
>
> Reproducible by opening, binding, sending 8 bytes and than closing a
> socket. close() will block.
>
> static int j1939_sk_sendmsg(struct socket *sock, struct msghdr *msg,
> size_t size)
> {
> ...
>
> if (msg->msg_flags & MSG_SYN) {
> if (msg->msg_flags & MSG_DONTWAIT) {
> ret = j1939_sock_pending_add_first(&jsk->sk);
> if (ret > 0)
> ret = -EAGAIN;
> } else {
> ret = wait_event_interruptible(jsk->waitq,
> j1939_sock_pending_add_first(&jsk->sk));
> }
> if (ret < 0)
> goto put_dev;
> } else {
> j1939_sock_pending_add(&jsk->sk); <--- incremented regardless of size
> }
>
> priv = j1939_priv_get_by_ndev(ndev);
> if (!priv)
> return -EINVAL;
>
> if (size > 8)
> /* re-route via transport protocol */
> ret = j1939_sk_send_multi(priv, sk, msg, size);
> else
> ret = j1939_sk_send_one(priv, sk, msg, size);
>
> j1939_priv_put(priv);
> if (ret < 0)
> j1939_sock_pending_del(&jsk->sk); <--- decremented on error only
>
> ...
>
>
> The jsk->skb_pending counter is also incremented for regular (non
> transport) sends.
Good point!
I'm still on other project. Do you wont to provide a patch?
Probably j1939_sock_pending_del() should be handled in
j1939_sk_send_multi() and j1939_sk_send_one().
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
