* Re: [PATCH net-next] sctp: check dst 'NULL' before use it in, sctp_packet_transmit
2013-12-25 7:47 [PATCH net-next] sctp: check dst 'NULL' before use it in, sctp_packet_transmit Wang Weidong
@ 2013-12-25 10:24 ` Daniel Borkmann
2013-12-25 10:25 ` Daniel Borkmann
2013-12-26 5:55 ` [PATCH net-next v2] sctp: move skb_dst_set() a bit downwards in sctp_packet_transmit() Wang Weidong
2 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2013-12-25 10:24 UTC (permalink / raw)
To: Wang Weidong
Cc: Neil Horman, Vlad Yasevich, David Miller, netdev,
linux-sctp@vger.kernel.org
On 12/25/2013 08:47 AM, Wang Weidong wrote:
> skb_dst_set will use dst, if dst is NULL although is not a problem,
> then goto the no_route and free nskb, so do the skb_dst_set is pointless.
> so check dst before use it. Remove the unnecessary initialization as well.
Please also cc linux-sctp as you did before!
Just went through the code, only reading from your subject title it first
sounded like a NULL pointer dereference, but in fact the code is fine and
nothing is wrong with it. I'd suggest you should make the subject sound
more "harmless" to not confuse people, imho, since all you do here is some
cleanup and rearrangement. "Use" sounds to me as dereferencing dst that is
NULL.
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---
> net/sctp/output.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 3be70a4..9b76d62 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -387,7 +387,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)
> int err = 0;
> int padding; /* How much padding do we need? */
> __u8 has_data = 0;
> - struct dst_entry *dst = tp->dst;
> + struct dst_entry *dst;
> unsigned char *auth = NULL; /* pointer to auth in skb data */
>
> pr_debug("%s: packet:%p\n", __func__, packet);
> @@ -420,9 +420,9 @@ int sctp_packet_transmit(struct sctp_packet *packet)
> }
> }
> dst = dst_clone(tp->dst);
> - skb_dst_set(nskb, dst);
> if (!dst)
> goto no_route;
Nit: you should set a newline here.
> + skb_dst_set(nskb, dst);
>
> /* Build the SCTP header. */
> sh = (struct sctphdr *)skb_push(nskb, sizeof(struct sctphdr));
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] sctp: check dst 'NULL' before use it in, sctp_packet_transmit
2013-12-25 7:47 [PATCH net-next] sctp: check dst 'NULL' before use it in, sctp_packet_transmit Wang Weidong
2013-12-25 10:24 ` Daniel Borkmann
@ 2013-12-25 10:25 ` Daniel Borkmann
2013-12-25 12:25 ` Wang Weidong
2013-12-26 5:55 ` [PATCH net-next v2] sctp: move skb_dst_set() a bit downwards in sctp_packet_transmit() Wang Weidong
2 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2013-12-25 10:25 UTC (permalink / raw)
To: Wang Weidong
Cc: Neil Horman, Vlad Yasevich, David Miller, netdev,
linux-sctp@vger.kernel.org
On 12/25/2013 08:47 AM, Wang Weidong wrote:
> skb_dst_set will use dst, if dst is NULL although is not a problem,
> then goto the no_route and free nskb, so do the skb_dst_set is pointless.
> so check dst before use it. Remove the unnecessary initialization as well.
Please also cc linux-sctp as you did before!
Just went through the code, only reading from your subject title it first
sounded like a NULL pointer dereference, but in fact the code is fine and
nothing is wrong with it. I'd suggest you should make the subject sound
more "harmless" to not confuse people, imho, since all you do here is some
cleanup and rearrangement. "Use" sounds to me as dereferencing dst that is
NULL.
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---
> net/sctp/output.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 3be70a4..9b76d62 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -387,7 +387,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)
> int err = 0;
> int padding; /* How much padding do we need? */
> __u8 has_data = 0;
> - struct dst_entry *dst = tp->dst;
> + struct dst_entry *dst;
> unsigned char *auth = NULL; /* pointer to auth in skb data */
>
> pr_debug("%s: packet:%p\n", __func__, packet);
> @@ -420,9 +420,9 @@ int sctp_packet_transmit(struct sctp_packet *packet)
> }
> }
> dst = dst_clone(tp->dst);
> - skb_dst_set(nskb, dst);
> if (!dst)
> goto no_route;
Nit: you should set a newline here.
> + skb_dst_set(nskb, dst);
>
> /* Build the SCTP header. */
> sh = (struct sctphdr *)skb_push(nskb, sizeof(struct sctphdr));
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] sctp: check dst 'NULL' before use it in, sctp_packet_transmit
2013-12-25 10:25 ` Daniel Borkmann
@ 2013-12-25 12:25 ` Wang Weidong
0 siblings, 0 replies; 6+ messages in thread
From: Wang Weidong @ 2013-12-25 12:25 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Neil Horman, Vlad Yasevich, David Miller, netdev,
linux-sctp@vger.kernel.org
From: Wang Weidong <wangweidong1@huawei.com>
On 2013/12/25 18:25, Daniel Borkmann wrote:
> On 12/25/2013 08:47 AM, Wang Weidong wrote:
>> skb_dst_set will use dst, if dst is NULL although is not a problem,
>> then goto the no_route and free nskb, so do the skb_dst_set is pointless.
>> so check dst before use it. Remove the unnecessary initialization as well.
>
> Please also cc linux-sctp as you did before!
>
> Just went through the code, only reading from your subject title it first
> sounded like a NULL pointer dereference, but in fact the code is fine and
> nothing is wrong with it. I'd suggest you should make the subject sound
> more "harmless" to not confuse people, imho, since all you do here is some
> cleanup and rearrangement. "Use" sounds to me as dereferencing dst that is
> NULL.
>
Sorry for that. I will fix the subject title and cc to linux-sctp.
Regards,
Wang
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
>> net/sctp/output.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sctp/output.c b/net/sctp/output.c
>> index 3be70a4..9b76d62 100644
>> --- a/net/sctp/output.c
>> +++ b/net/sctp/output.c
>> @@ -387,7 +387,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)
>> int err = 0;
>> int padding; /* How much padding do we need? */
>> __u8 has_data = 0;
>> - struct dst_entry *dst = tp->dst;
>> + struct dst_entry *dst;
>> unsigned char *auth = NULL; /* pointer to auth in skb data */
>>
>> pr_debug("%s: packet:%p\n", __func__, packet);
>> @@ -420,9 +420,9 @@ int sctp_packet_transmit(struct sctp_packet *packet)
>> }
>> }
>> dst = dst_clone(tp->dst);
>> - skb_dst_set(nskb, dst);
>> if (!dst)
>> goto no_route;
>
> Nit: you should set a newline here.
>
Nice. Thanks.
>> + skb_dst_set(nskb, dst);
>>
>> /* Build the SCTP header. */
>> sh = (struct sctphdr *)skb_push(nskb, sizeof(struct sctphdr));
>>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net-next v2] sctp: move skb_dst_set() a bit downwards in sctp_packet_transmit()
2013-12-25 7:47 [PATCH net-next] sctp: check dst 'NULL' before use it in, sctp_packet_transmit Wang Weidong
2013-12-25 10:24 ` Daniel Borkmann
2013-12-25 10:25 ` Daniel Borkmann
@ 2013-12-26 5:55 ` Wang Weidong
2014-01-02 14:58 ` Neil Horman
2 siblings, 1 reply; 6+ messages in thread
From: Wang Weidong @ 2013-12-26 5:55 UTC (permalink / raw)
To: Neil Horman, Vlad Yasevich, David Miller
Cc: Daniel Borkmann, netdev, linux-sctp
skb_dst_set will use dst, if dst is NULL although is not a problem,
then goto the 'no_route' and free nskb, so do the skb_dst_set is pointless.
so move the skb_dst_set after dst check.
Remove the unnecessary initialization as well.
v2: fix the subject line because it would confuse people,
as pointed out by Daniel.
Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
net/sctp/output.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sctp/output.c b/net/sctp/output.c
index 3be70a4..9b76d62 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -387,7 +387,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)
int err = 0;
int padding; /* How much padding do we need? */
__u8 has_data = 0;
- struct dst_entry *dst = tp->dst;
+ struct dst_entry *dst;
unsigned char *auth = NULL; /* pointer to auth in skb data */
pr_debug("%s: packet:%p\n", __func__, packet);
@@ -420,9 +420,9 @@ int sctp_packet_transmit(struct sctp_packet *packet)
}
}
dst = dst_clone(tp->dst);
- skb_dst_set(nskb, dst);
if (!dst)
goto no_route;
+ skb_dst_set(nskb, dst);
/* Build the SCTP header. */
sh = (struct sctphdr *)skb_push(nskb, sizeof(struct sctphdr));
--
1.7.12
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next v2] sctp: move skb_dst_set() a bit downwards in sctp_packet_transmit()
2013-12-26 5:55 ` [PATCH net-next v2] sctp: move skb_dst_set() a bit downwards in sctp_packet_transmit() Wang Weidong
@ 2014-01-02 14:58 ` Neil Horman
0 siblings, 0 replies; 6+ messages in thread
From: Neil Horman @ 2014-01-02 14:58 UTC (permalink / raw)
To: Wang Weidong
Cc: Vlad Yasevich, David Miller, Daniel Borkmann, netdev, linux-sctp
On Thu, Dec 26, 2013 at 01:55:56PM +0800, Wang Weidong wrote:
> skb_dst_set will use dst, if dst is NULL although is not a problem,
> then goto the 'no_route' and free nskb, so do the skb_dst_set is pointless.
> so move the skb_dst_set after dst check.
> Remove the unnecessary initialization as well.
>
> v2: fix the subject line because it would confuse people,
> as pointed out by Daniel.
>
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
> ---
> net/sctp/output.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 3be70a4..9b76d62 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -387,7 +387,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)
> int err = 0;
> int padding; /* How much padding do we need? */
> __u8 has_data = 0;
> - struct dst_entry *dst = tp->dst;
> + struct dst_entry *dst;
> unsigned char *auth = NULL; /* pointer to auth in skb data */
>
> pr_debug("%s: packet:%p\n", __func__, packet);
> @@ -420,9 +420,9 @@ int sctp_packet_transmit(struct sctp_packet *packet)
> }
> }
> dst = dst_clone(tp->dst);
> - skb_dst_set(nskb, dst);
> if (!dst)
> goto no_route;
> + skb_dst_set(nskb, dst);
>
> /* Build the SCTP header. */
> sh = (struct sctphdr *)skb_push(nskb, sizeof(struct sctphdr));
> --
> 1.7.12
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread