On wo, 23 jan 2019 13:01:11 +0100, Oleksij Rempel wrote:
> This is dangerous behavior, especially if it is done by default. If we
> still needed it, it can be added after upstreaming.
In a true (?) j1939 program, you don't care about the precise packet size
you're sending. If you then send 2 subsequent packets, 1 with TP and 1
without, then the 2nd will likely arrive sooner than the first, which
is probably not what you want.
The piece of code you're about to remove increments will set
jsk->skb_pending only if it was 0, atomically.
If it wasn't 0, then send() will block or fail.
Can you explain how this is dangerous?
IMHO, it is not done by default, but ony when MSG_SYN is given during send().
How do you think that synchronous send is done by default?
Kurt
>
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> ---
> net/can/j1939/socket.c | 33 +--------------------------------
> 1 file changed, 1 insertion(+), 32 deletions(-)
>
> diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
> index a313077ca80a..38e2fb47292b 100644
> --- a/net/can/j1939/socket.c
> +++ b/net/can/j1939/socket.c
> @@ -73,23 +73,6 @@ static inline bool j1939_pgn_is_clean_pdu(pgn_t pgn)
> return true;
> }
>
> -/* j1939_sock_pending_add_first
> - * Succeeds when the first pending SKB is scheduled
> - * Fails when SKB are already pending
> - */
> -static inline int j1939_sock_pending_add_first(struct sock *sk)
> -{
> - struct j1939_sock *jsk = j1939_sk(sk);
> -
> - /* atomic_cmpxchg returns the old value
> - * When it was 0, it is exchanged with 1 and this function
> - * succeeded. (return 1)
> - * When it was != 0, it is not exchanged, and this function
> - * fails (returns 0).
> - */
> - return !atomic_cmpxchg(&jsk->skb_pending, 0, 1);
> -}
> -
> static inline void j1939_sock_pending_add(struct sock *sk)
> {
> struct j1939_sock *jsk = j1939_sk(sk);
> @@ -773,25 +756,11 @@ static int j1939_sk_sendmsg(struct socket *sock, struct msghdr *msg,
> if (!ndev)
> return -ENXIO;
>
> - 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);
> - }
> -
> priv = j1939_priv_get_by_ndev(ndev);
> if (!priv)
> return -EINVAL;
>
> + j1939_sock_pending_add(&jsk->sk);
> if (size > 8)
> /* re-route via transport protocol */
> ret = j1939_sk_send_multi(priv, sk, msg, size);
> --
> 2.20.1
>
