netdev - Re: [PATCH net 2/3] pktgen: give user result when disable vlan/svlan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 15 May 2014 22:12:10 +0800
From:	Hangbin Liu <liuhangbin@...il.com>
To:	Daniel Borkmann <dborkman@...hat.com>
Cc:	David Miller <davem@...emloft.net>,
	Francesco Fondelli <francesco.fondelli@...il.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net 2/3] pktgen: give user result when disable vlan/svlan

On Thu, May 15, 2014 at 07:05:56PM +0800, Hangbin Liu wrote:
> On Thu, May 15, 2014 at 12:00:31PM +0200, Daniel Borkmann wrote:
> > On 05/15/2014 11:46 AM, Hangbin Liu wrote:
> > >Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> > >---
> > >  net/core/pktgen.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > >diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> > >index dcf367f..1809bdf 100644
> > >--- a/net/core/pktgen.c
> > >+++ b/net/core/pktgen.c
> > >@@ -1573,8 +1573,7 @@ static ssize_t pktgen_if_write(struct file *file,
> > >  			pkt_dev->vlan_id = 0xffff; /* turn off VLAN/SVLAN */
> > >  			pkt_dev->svlan_id = 0xffff;
> > >
> > >-			if (debug)
> > >-				pr_debug("VLAN/SVLAN turned off\n");
> > >+			sprintf(pg_result, "OK: VLAN/SVLAN turned off");
> > 
> > I think that might break user scripts as pg_result is copied to user
> > space, and currently only expected to return 'OK: svlan_id=%u' if it
> > was actually successful. Unfortunately, scripts that might only check
> > for 'OK' in the string could make wrong assumptions later on.

or maybe we can return 0xffff when disabled vlan? like

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index fabd17e..64afb91 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1580,7 +1580,10 @@ static ssize_t pktgen_if_write(struct file *file,
                        pkt_dev->vlan_id = 0xffff; /* turn off VLAN/SVLAN */
                        pkt_dev->svlan_id = 0xffff;

-                       sprintf(pg_result, "OK: VLAN/SVLAN turned off");
+                       sprintf(pg_result, "OK: vlan_id=0x%x", pkt_dev->vlan_id);
+
+                       if (debug)
+                               pr_debug("VLAN/SVLAN turned off\n");
                }
                return count;
        }

But I saw dst_mac, src_mac and clear_counters also didn't use the OK: param=value
format.
sprintf(pg_result, "OK: dstmac %pM", pkt_dev->dst_mac);
sprintf(pg_result, "OK: srcmac %pM", pkt_dev->src_mac);
sprintf(pg_result, "OK: Clearing counters.\n");

We can change src_mac and dst_mac to "srcmac=%pM" easily, but I don't know how
to edit counters. return OK: counters=0 ?

> 
> But the original behavior will keep the last return value, which may make user
> confused. e.g.
> 
> # echo vlan_id 10 > /proc/net/pktgen/eno4
> # cat /proc/net/pktgen/eno4
> Params: count 1000  min_pkt_size: 0  max_pkt_size: 0
>      frags: 0  delay: 0  clone_skb: 0  ifname: eno4
>      flows: 0 flowlen: 0
>      queue_map_min: 0  queue_map_max: 0
>      dst_min:   dst_max:
>         src_min:   src_max:
>      src_mac: 6c:ae:8b:20:7b:cc dst_mac: 00:00:00:00:00:00
>      udp_src_min: 9  udp_src_max: 9  udp_dst_min: 9  udp_dst_max: 9
>      src_mac_count: 0  dst_mac_count: 0
>      vlan_id: 10  vlan_p: 0  vlan_cfi: 0
>      Flags:
> Current:
>      pkts-sofar: 0  errors: 0
>      started: 0us  stopped: 0us idle: 0us
>      seq_num: 0  cur_dst_mac_offset: 0  cur_src_mac_offset: 0
>      cur_saddr: 0.0.0.0  cur_daddr: 0.0.0.0
>      cur_udp_dst: 0  cur_udp_src: 0
>      cur_queue_map: 0
>      flows: 0
> Result: OK: vlan_id=10
> 
> # echo vlan_id 9999 > /proc/net/pktgen/eno4
> # cat /proc/net/pktgen/eno4
> Params: count 1000  min_pkt_size: 0  max_pkt_size: 0
>      frags: 0  delay: 0  clone_skb: 0  ifname: eno4
>      flows: 0 flowlen: 0
>      queue_map_min: 0  queue_map_max: 0
>      dst_min:   dst_max:
>         src_min:   src_max:
>      src_mac: 6c:ae:8b:20:7b:cc dst_mac: 00:00:00:00:00:00
>      udp_src_min: 9  udp_src_max: 9  udp_dst_min: 9  udp_dst_max: 9
>      src_mac_count: 0  dst_mac_count: 0
>      Flags:
> Current:
>      pkts-sofar: 0  errors: 0
>      started: 0us  stopped: 0us idle: 0us
>      seq_num: 0  cur_dst_mac_offset: 0  cur_src_mac_offset: 0
>      cur_saddr: 0.0.0.0  cur_daddr: 0.0.0.0
>      cur_udp_dst: 0  cur_udp_src: 0
>      cur_queue_map: 0
>      flows: 0
> Result: OK: vlan_id=10
> 
> > 
> > >  		}
> > >  		return count;
> > >  	}
> > >@@ -1629,8 +1628,7 @@ static ssize_t pktgen_if_write(struct file *file,
> > >  		} else {
> > >  			pkt_dev->svlan_id = 0xffff;
> > >
> > >-			if (debug)
> > >-				pr_debug("SVLAN turned off\n");
> > >+			sprintf(pg_result, "OK: SVLAN turned off");
> > 
> > Ditto.
> > 
> > >  		}
> > >  		return count;
> > >  	}
> > >
> 
> -- 
> 
> Thanks & Best Regards
> Hangbin Liu <liuhangbin@...il.com>

-- 

Thanks & Best Regards
Hangbin Liu <liuhangbin@...il.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html