| Message ID | 20171108072142.30870-4-saeedm@mellanox.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,1/6] net/mlx5: Loop over temp list to release delay events | expand |
On Wed, Nov 8, 2017 at 4:21 PM, Saeed Mahameed <saeedm@mellanox.com> wrote: > From: Kamal Heib <kamalh@mellanox.com> > > In the current code, if a size of zero is passed to > mlx5_fpga_mem_{read|write}_i2c() functions the "err" Don't we need to fix the call site where zero size is provided and not in called function? Isn't sending down a zero size a sign for a bug which we are not fixing? > return value will not initialized. > > Fixes: a9956d35d199 ('net/mlx5: FPGA, Add SBU infrastructure') > Signed-off-by: Kamal Heib <kamalh@mellanox.com> > Reviewed-by: Yevgeny Kliteynik <kliteyn@mellanox.com> > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
On Wed, 2017-11-08 at 23:13 +0900, Or Gerlitz wrote: > On Wed, Nov 8, 2017 at 4:21 PM, Saeed Mahameed <saeedm@mellanox.com> > wrote: > > From: Kamal Heib <kamalh@mellanox.com> > > > > In the current code, if a size of zero is passed to > > mlx5_fpga_mem_{read|write}_i2c() functions the "err" > > Don't we need to fix the call site where zero size is provided and > not > in called function? > Isn't sending down a zero size a sign for a bug which we are not fixing? > Both functions are called from an exported symbols. so I think the size validation should be within this two functions just like the case of checking that mdev isn't set. > > return value will not initialized. > > > > Fixes: a9956d35d199 ('net/mlx5: FPGA, Add SBU infrastructure') > > Signed-off-by: Kamal Heib <kamalh@mellanox.com> > > Reviewed-by: Yevgeny Kliteynik <kliteyn@mellanox.com> > > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
On Thu, Nov 9, 2017 at 4:43 PM, Kamal Heib <kamalh@mellanox.com> wrote: > On Wed, 2017-11-08 at 23:13 +0900, Or Gerlitz wrote: >> On Wed, Nov 8, 2017 at 4:21 PM, Saeed Mahameed <saeedm@mellanox.com> >> wrote: >> > From: Kamal Heib <kamalh@mellanox.com> >> > >> > In the current code, if a size of zero is passed to >> > mlx5_fpga_mem_{read|write}_i2c() functions the "err" >> >> Don't we need to fix the call site where zero size is provided and >> not >> in called function? >> > Isn't sending down a zero size a sign for a bug which we are not > fixing? >> > Both functions are called from an exported symbols. so I think the size > validation should be within this two functions just like the case of > checking that mdev isn't set. mmm, I see exported to who exactly? how are they being called, by func pointer? can you point to the call sites?
On Thu, Nov 9, 2017 at 4:43 PM, Kamal Heib <kamalh@mellanox.com> wrote: > On Wed, 2017-11-08 at 23:13 +0900, Or Gerlitz wrote: >> On Wed, Nov 8, 2017 at 4:21 PM, Saeed Mahameed <saeedm@mellanox.com> >> wrote: >> > From: Kamal Heib <kamalh@mellanox.com> >> > >> > In the current code, if a size of zero is passed to >> > mlx5_fpga_mem_{read|write}_i2c() functions the "err" >> >> Don't we need to fix the call site where zero size is provided and >> not >> in called function? >> > Isn't sending down a zero size a sign for a bug which we are not > fixing? >> > Both functions are called from an exported symbols. so I think the size > validation should be within this two functions just like the case of > checking that mdev isn't set. Note that the kernel trust model doesn't enforce you to check everything as you go.
On Thu, 2017-11-09 at 18:12 +0900, Or Gerlitz wrote: > On Thu, Nov 9, 2017 at 4:43 PM, Kamal Heib <kamalh@mellanox.com> > wrote: > > On Wed, 2017-11-08 at 23:13 +0900, Or Gerlitz wrote: > > > On Wed, Nov 8, 2017 at 4:21 PM, Saeed Mahameed <saeedm@mellanox.c > > > om> > > > wrote: > > > > From: Kamal Heib <kamalh@mellanox.com> > > > > > > > > In the current code, if a size of zero is passed to > > > > mlx5_fpga_mem_{read|write}_i2c() functions the "err" > > > > > > Don't we need to fix the call site where zero size is provided > > > and > > > not > > > in called function? > > > > > > > Isn't sending down a zero size a sign for a bug which we are not > > fixing? > > > > > > > Both functions are called from an exported symbols. so I think the > > size > > validation should be within this two functions just like the case > > of > > checking that mdev isn't set. > > mmm, I see exported to who exactly? how are they being called, by > func pointer? > can you point to the call sites? Or, are you ok with this patch ? I would like to post V2 with the reviewed-by tag fix.
On Fri, Nov 10, 2017 at 3:13 PM, Saeed Mahameed <saeedm@mellanox.com> wrote: > On Thu, 2017-11-09 at 18:12 +0900, Or Gerlitz wrote: >> On Thu, Nov 9, 2017 at 4:43 PM, Kamal Heib <kamalh@mellanox.com> >> wrote: >> > On Wed, 2017-11-08 at 23:13 +0900, Or Gerlitz wrote: >> > > On Wed, Nov 8, 2017 at 4:21 PM, Saeed Mahameed <saeedm@mellanox.c >> > > om> >> > > wrote: >> > > > From: Kamal Heib <kamalh@mellanox.com> >> > > > >> > > > In the current code, if a size of zero is passed to >> > > > mlx5_fpga_mem_{read|write}_i2c() functions the "err" >> > > >> > > Don't we need to fix the call site where zero size is provided >> > > and >> > > not >> > > in called function? >> > > >> > >> > Isn't sending down a zero size a sign for a bug which we are not >> > fixing? >> > > >> > >> > Both functions are called from an exported symbols. so I think the >> > size >> > validation should be within this two functions just like the case >> > of >> > checking that mdev isn't set. >> >> mmm, I see exported to who exactly? how are they being called, by >> func pointer? >> can you point to the call sites? > > Or, are you ok with this patch ? I would like to post V2 with the > reviewed-by tag fix. The RB tag issue was on another patch.. for this patch I realized after talking to the author that it comes to fix a build warning. I would be happy if we can clarify that in the change log.
On Fri, 2017-11-10 at 15:23 +0900, Or Gerlitz wrote: > On Fri, Nov 10, 2017 at 3:13 PM, Saeed Mahameed <saeedm@mellanox.com> > wrote: > > On Thu, 2017-11-09 at 18:12 +0900, Or Gerlitz wrote: > > > On Thu, Nov 9, 2017 at 4:43 PM, Kamal Heib <kamalh@mellanox.com> > > > wrote: > > > > On Wed, 2017-11-08 at 23:13 +0900, Or Gerlitz wrote: > > > > > On Wed, Nov 8, 2017 at 4:21 PM, Saeed Mahameed <saeedm@mellan > > > > > ox.c > > > > > om> > > > > > wrote: > > > > > > From: Kamal Heib <kamalh@mellanox.com> > > > > > > > > > > > > In the current code, if a size of zero is passed to > > > > > > mlx5_fpga_mem_{read|write}_i2c() functions the "err" > > > > > > > > > > Don't we need to fix the call site where zero size is > > > > > provided > > > > > and > > > > > not > > > > > in called function? > > > > > > > > > > > > > Isn't sending down a zero size a sign for a bug which we are > > > > not > > > > fixing? > > > > > > > > > > > > > Both functions are called from an exported symbols. so I think > > > > the > > > > size > > > > validation should be within this two functions just like the > > > > case > > > > of > > > > checking that mdev isn't set. > > > > > > mmm, I see exported to who exactly? how are they being called, by > > > func pointer? > > > can you point to the call sites? > > > > Or, are you ok with this patch ? I would like to post V2 with the > > reviewed-by tag fix. > > The RB tag issue was on another patch.. for this patch I realized > after talking > to the author that it comes to fix a build warning. I would be happy > if we can clarify > that in the change log. Ok I will drop this patch until the author provides the missing information.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c index 3c11d6e2160a..14962969c5ba 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/sdk.c @@ -66,6 +66,9 @@ static int mlx5_fpga_mem_read_i2c(struct mlx5_fpga_device *fdev, size_t size, u8 actual_size; int err; + if (!size) + return -EINVAL; + if (!fdev->mdev) return -ENOTCONN; @@ -95,6 +98,9 @@ static int mlx5_fpga_mem_write_i2c(struct mlx5_fpga_device *fdev, size_t size, u8 actual_size; int err; + if (!size) + return -EINVAL; + if (!fdev->mdev) return -ENOTCONN;