Re: [PATCH net-next 11/14] rxrpc: Make rxrpc_send_packet() take a connection not a transport [ver #2]

[Geert Uytterhoeven]

Hi David,

On Wed, Jun 22, 2016 at 6:04 PM, David Howells <dhowells@xxxxxxxxxx> wrote:
> Make rxrpc_send_packet() take a connection not a transport as part of the
> phasing out of the rxrpc_transport struct.
>
> Whilst we're at it, rename the function to rxrpc_send_data_packet() to
> differentiate it from the other packet sending functions.
>
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>

This is now upstream commit 985a5c824a52e9f7

> --- a/net/rxrpc/output.c
> +++ b/net/rxrpc/output.c
> @@ -338,7 +338,7 @@ EXPORT_SYMBOL(rxrpc_kernel_abort_call);
>  /*
>   * send a packet through the transport endpoint
>   */
> -int rxrpc_send_packet(struct rxrpc_transport *trans, struct sk_buff *skb)
> +int rxrpc_send_data_packet(struct rxrpc_connection *conn, struct sk_buff *skb)
>  {
>         struct kvec iov[1];
>         struct msghdr msg;
> @@ -349,30 +349,30 @@ int rxrpc_send_packet(struct rxrpc_transport *trans, struct sk_buff *skb)

net/rxrpc/output.c: In function ârxrpc_send_data_packetâ:
net/rxrpc/output.c:252: warning: âretâ may be used uninitialized in
this function
(line number is from current mainline)

>         iov[0].iov_base = skb->head;
>         iov[0].iov_len = skb->len;
>
> -       msg.msg_name = &trans->peer->srx.transport.sin;
> -       msg.msg_namelen = sizeof(trans->peer->srx.transport.sin);
> +       msg.msg_name = &conn->params.peer->srx.transport;
> +       msg.msg_namelen = conn->params.peer->srx.transport_len;
>         msg.msg_control = NULL;
>         msg.msg_controllen = 0;
>         msg.msg_flags = 0;
>
>         /* send the packet with the don't fragment bit set if we currently
>          * think it's small enough */
> -       if (skb->len - sizeof(struct rxrpc_wire_header) < trans->peer->maxdata) {
> -               down_read(&trans->local->defrag_sem);
> +       if (skb->len - sizeof(struct rxrpc_wire_header) < conn->params.peer->maxdata) {
> +               down_read(&conn->params.local->defrag_sem);

If this branch is not taken...

>                 /* send the packet by UDP
>                  * - returns -EMSGSIZE if UDP would have to fragment the packet
>                  *   to go out of the interface
>                  *   - in which case, we'll have processed the ICMP error
>                  *     message and update the peer record
>                  */
> -               ret = kernel_sendmsg(trans->local->socket, &msg, iov, 1,
> +               ret = kernel_sendmsg(conn->params.local->socket, &msg, iov, 1,
>                                      iov[0].iov_len);
>
> -               up_read(&trans->local->defrag_sem);
> +               up_read(&conn->params.local->defrag_sem);
>                 if (ret == -EMSGSIZE)
>                         goto send_fragmentable;
>
> -               _leave(" = %d [%u]", ret, trans->peer->maxdata);
> +               _leave(" = %d [%u]", ret, conn->params.peer->maxdata);
>                 return ret;
>         }
>
> @@ -380,21 +380,28 @@ send_fragmentable:
>         /* attempt to send this message with fragmentation enabled */
>         _debug("send fragment");
>
> -       down_write(&trans->local->defrag_sem);
> -       opt = IP_PMTUDISC_DONT;
> -       ret = kernel_setsockopt(trans->local->socket, SOL_IP, IP_MTU_DISCOVER,
> -                               (char *) &opt, sizeof(opt));
> -       if (ret == 0) {
> -               ret = kernel_sendmsg(trans->local->socket, &msg, iov, 1,
> -                                    iov[0].iov_len);
> -
> -               opt = IP_PMTUDISC_DO;
> -               kernel_setsockopt(trans->local->socket, SOL_IP,
> -                                 IP_MTU_DISCOVER, (char *) &opt, sizeof(opt));
> +       down_write(&conn->params.local->defrag_sem);
> +
> +       switch (conn->params.local->srx.transport.family) {
> +       case AF_INET:
> +               opt = IP_PMTUDISC_DONT;
> +               ret = kernel_setsockopt(conn->params.local->socket,
> +                                       SOL_IP, IP_MTU_DISCOVER,
> +                                       (char *)&opt, sizeof(opt));
> +               if (ret == 0) {
> +                       ret = kernel_sendmsg(conn->params.local->socket, &msg, iov, 1,
> +                                            iov[0].iov_len);
> +
> +                       opt = IP_PMTUDISC_DO;
> +                       kernel_setsockopt(conn->params.local->socket, SOL_IP,
> +                                         IP_MTU_DISCOVER,
> +                                         (char *)&opt, sizeof(opt));
> +               }
> +               break;

... and none of the cases (current upstream also has AF_INET6 if
CONFIG_AF_RXRPC_IPV6 is enabled) match ...

>         }
>
> -       up_write(&trans->local->defrag_sem);
> -       _leave(" = %d [frag %u]", ret, trans->peer->maxdata);
> +       up_write(&conn->params.local->defrag_sem);
> +       _leave(" = %d [frag %u]", ret, conn->params.peer->maxdata);
>         return ret;

... then ret is not initialized.

I didn't create a patch, as I'm not sure this is a false positive or not.
Is it possible that none of the cases match?

>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds