Re: [PATCH] examples/ptpclient: fix delay request message

[David Marchand <david.marchand@redhat.com>]

Hello,

On Wed, Nov 17, 2021 at 7:19 AM <vanshika.shukla@nxp.com> wrote:
>
> From: Vanshika Shukla <vanshika.shukla@nxp.com>
>
> The size of delay request message sent out by the DPDK
> ptpclient application was observed to have extra length
> than expected. Due to this, bad messages were observed
> on the master side and delay response was not received.
>
> This patch fixes this bug.

We need a Fixes: line and it is likely a candidate for backport.

>
> Signed-off-by: Vanshika Shukla <vanshika.shukla@nxp.com>
> ---
>  examples/ptpclient/ptpclient.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/examples/ptpclient/ptpclient.c b/examples/ptpclient/ptpclient.c
> index 4f32ade7fb..1eb813ab01 100644
> --- a/examples/ptpclient/ptpclient.c
> +++ b/examples/ptpclient/ptpclient.c
> @@ -422,7 +422,7 @@ parse_fup(struct ptpv2_data_slave_ordinary *ptp_data)
>
>                 created_pkt = rte_pktmbuf_alloc(mbuf_pool);
>                 pkt_size = sizeof(struct rte_ether_hdr) +
> -                       sizeof(struct ptp_message);
> +                       sizeof(struct delay_req_msg);
>                 created_pkt->data_len = pkt_size;
>                 created_pkt->pkt_len = pkt_size;

This example code is in a bad shape.

Directly updating mbuf fields without caring for available data size
is a bad practice, plus there is no check on available size in
allocated buffer.
It should be like (untested):

                created_pkt = rte_pktmbuf_alloc(mbuf_pool);
                pkt_size = sizeof(struct rte_ether_hdr) +
-                       sizeof(struct ptp_message);
-               created_pkt->data_len = pkt_size;
-               created_pkt->pkt_len = pkt_size;
+                       sizeof(struct delay_req_msg);
+               if (rte_pktmbuf_append(created_pkt, pkt_size) == NULL) {
+                       rte_pktmbuf_free(created_pkt);
+                       return;
+               }
+

After this, the rest of the code uses a struct ptp_msg pointer.
It should use a pointer with the right type.

Something like:

+               struct delay_req_msg *req_msg;
...
-               ptp_msg = (struct ptp_message *)
-                       (rte_pktmbuf_mtod(created_pkt, char *) +
-                       sizeof(struct rte_ether_hdr));
+               req_msg = rte_pktmbuf_mtod_offset(created_pkt,
+                       struct delay_req_msg *, sizeof(struct rte_ether_hdr));

etc...


-- 
David Marchand