| Message ID | 1422881031-25105-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | Accepted |
| Commit | 3be08a35e2f2161ed19e4a36ba77800a44286625 |
| Headers | show |
That patch was not reviewed. Maxim. On 2 February 2015 at 15:43, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > CID: 85427 > https://bugs.linaro.org/show_bug.cgi?id=1175 > get_pktio_entry() can return NULL entry then it can be derefenced > inside is_free(entry). > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > platform/linux-generic/include/odp_packet_io_internal.h | 8 ++++++-- > platform/linux-generic/odp_packet_io.c | 2 +- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/platform/linux-generic/include/odp_packet_io_internal.h > b/platform/linux-generic/include/odp_packet_io_internal.h > index 488818c..2c766f0 100644 > --- a/platform/linux-generic/include/odp_packet_io_internal.h > +++ b/platform/linux-generic/include/odp_packet_io_internal.h > @@ -22,6 +22,7 @@ extern "C" { > #include <odp_packet_socket.h> > #include <odp_classification_datamodel.h> > #include <odp_align_internal.h> > +#include <odp_debug_internal.h> > > #include <odp/config.h> > #include <odp/hints.h> > @@ -67,10 +68,13 @@ extern void *pktio_entry_ptr[]; > > static inline pktio_entry_t *get_pktio_entry(odp_pktio_t id) > { > - if (odp_unlikely(id == ODP_PKTIO_INVALID || > - id > ODP_CONFIG_PKTIO_ENTRIES)) > + if (odp_unlikely(id == ODP_PKTIO_INVALID)) > return NULL; > > + if (odp_unlikely(id > ODP_CONFIG_PKTIO_ENTRIES)) > + ODP_ABORT("pktio limit %d/%d exceed\n", > + id, ODP_CONFIG_PKTIO_ENTRIES); > + > return pktio_entry_ptr[id - 1]; > } > #ifdef __cplusplus > diff --git a/platform/linux-generic/odp_packet_io.c > b/platform/linux-generic/odp_packet_io.c > index bdb690e..cfb8d00 100644 > --- a/platform/linux-generic/odp_packet_io.c > +++ b/platform/linux-generic/odp_packet_io.c > @@ -320,7 +320,7 @@ odp_pktio_t odp_pktio_lookup(const char *dev) > > for (i = 1; i <= ODP_CONFIG_PKTIO_ENTRIES; ++i) { > entry = get_pktio_entry(i); > - if (is_free(entry)) > + if (!entry || is_free(entry)) > continue; > > lock_entry(entry); > -- > 1.8.5.1.163.gd7aced9 > >
Ping one more time. Maxim. On 02/16/2015 06:12 PM, Maxim Uvarov wrote: > That patch was not reviewed. > > Maxim. > > On 2 February 2015 at 15:43, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > CID: 85427 > https://bugs.linaro.org/show_bug.cgi?id=1175 > get_pktio_entry() can return NULL entry then it can be derefenced > inside is_free(entry). > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > platform/linux-generic/include/odp_packet_io_internal.h | 8 ++++++-- > platform/linux-generic/odp_packet_io.c | 2 +- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git > a/platform/linux-generic/include/odp_packet_io_internal.h > b/platform/linux-generic/include/odp_packet_io_internal.h > index 488818c..2c766f0 100644 > --- a/platform/linux-generic/include/odp_packet_io_internal.h > +++ b/platform/linux-generic/include/odp_packet_io_internal.h > @@ -22,6 +22,7 @@ extern "C" { > #include <odp_packet_socket.h> > #include <odp_classification_datamodel.h> > #include <odp_align_internal.h> > +#include <odp_debug_internal.h> > > #include <odp/config.h> > #include <odp/hints.h> > @@ -67,10 +68,13 @@ extern void *pktio_entry_ptr[]; > > static inline pktio_entry_t *get_pktio_entry(odp_pktio_t id) > { > - if (odp_unlikely(id == ODP_PKTIO_INVALID || > - id > ODP_CONFIG_PKTIO_ENTRIES)) > + if (odp_unlikely(id == ODP_PKTIO_INVALID)) > return NULL; > > + if (odp_unlikely(id > ODP_CONFIG_PKTIO_ENTRIES)) > + ODP_ABORT("pktio limit %d/%d exceed\n", > + id, ODP_CONFIG_PKTIO_ENTRIES); > + > return pktio_entry_ptr[id - 1]; > } > #ifdef __cplusplus > diff --git a/platform/linux-generic/odp_packet_io.c > b/platform/linux-generic/odp_packet_io.c > index bdb690e..cfb8d00 100644 > --- a/platform/linux-generic/odp_packet_io.c > +++ b/platform/linux-generic/odp_packet_io.c > @@ -320,7 +320,7 @@ odp_pktio_t odp_pktio_lookup(const char *dev) > > for (i = 1; i <= ODP_CONFIG_PKTIO_ENTRIES; ++i) { > entry = get_pktio_entry(i); > - if (is_free(entry)) > + if (!entry || is_free(entry)) > continue; > > lock_entry(entry); > -- > 1.8.5.1.163.gd7aced9 > >
On Wed, Feb 25, 2015 at 11:53 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Ping one more time. > > Maxim. > > On 02/16/2015 06:12 PM, Maxim Uvarov wrote: > >> That patch was not reviewed. >> >> Maxim. >> >> On 2 February 2015 at 15:43, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> CID: 85427 >> https://bugs.linaro.org/show_bug.cgi?id=1175 >> get_pktio_entry() can return NULL entry then it can be derefenced >> inside is_free(entry). >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> >> --- >> platform/linux-generic/include/odp_packet_io_internal.h | 8 ++++++-- >> platform/linux-generic/odp_packet_io.c | 2 +- >> 2 files changed, 7 insertions(+), 3 deletions(-) >> >> diff --git >> a/platform/linux-generic/include/odp_packet_io_internal.h >> b/platform/linux-generic/include/odp_packet_io_internal.h >> index 488818c..2c766f0 100644 >> --- a/platform/linux-generic/include/odp_packet_io_internal.h >> +++ b/platform/linux-generic/include/odp_packet_io_internal.h >> @@ -22,6 +22,7 @@ extern "C" { >> #include <odp_packet_socket.h> >> #include <odp_classification_datamodel.h> >> #include <odp_align_internal.h> >> +#include <odp_debug_internal.h> >> >> #include <odp/config.h> >> #include <odp/hints.h> >> @@ -67,10 +68,13 @@ extern void *pktio_entry_ptr[]; >> >> static inline pktio_entry_t *get_pktio_entry(odp_pktio_t id) >> { >> - if (odp_unlikely(id == ODP_PKTIO_INVALID || >> - id > ODP_CONFIG_PKTIO_ENTRIES)) >> + if (odp_unlikely(id == ODP_PKTIO_INVALID)) >> return NULL; >> >> + if (odp_unlikely(id > ODP_CONFIG_PKTIO_ENTRIES)) >> + ODP_ABORT("pktio limit %d/%d exceed\n", >> + id, ODP_CONFIG_PKTIO_ENTRIES); >> + >> > What is the advantage of introducing an ODP_ABORT() here? The previous check seemed equally safe and is just rejecting invalid parameters. > return pktio_entry_ptr[id - 1]; >> } >> #ifdef __cplusplus >> diff --git a/platform/linux-generic/odp_packet_io.c >> b/platform/linux-generic/odp_packet_io.c >> index bdb690e..cfb8d00 100644 >> --- a/platform/linux-generic/odp_packet_io.c >> +++ b/platform/linux-generic/odp_packet_io.c >> @@ -320,7 +320,7 @@ odp_pktio_t odp_pktio_lookup(const char *dev) >> >> for (i = 1; i <= ODP_CONFIG_PKTIO_ENTRIES; ++i) { >> entry = get_pktio_entry(i); >> - if (is_free(entry)) >> + if (!entry || is_free(entry)) >> > if (entry == NULL || is_free(entry)) might be clearer. > continue; >> >> lock_entry(entry); >> -- >> 1.8.5.1.163.gd7aced9 >> >> >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 02/25/2015 09:04 PM, Bill Fischofer wrote: > > > On Wed, Feb 25, 2015 at 11:53 AM, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: > > Ping one more time. > > Maxim. > > On 02/16/2015 06:12 PM, Maxim Uvarov wrote: > > That patch was not reviewed. > > Maxim. > > On 2 February 2015 at 15:43, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> wrote: > > CID: 85427 > https://bugs.linaro.org/show_bug.cgi?id=1175 > get_pktio_entry() can return NULL entry then it can be > derefenced > inside is_free(entry). > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> > > --- > platform/linux-generic/include/odp_packet_io_internal.h | > 8 ++++++-- > platform/linux-generic/odp_packet_io.c | 2 +- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git > a/platform/linux-generic/include/odp_packet_io_internal.h > b/platform/linux-generic/include/odp_packet_io_internal.h > index 488818c..2c766f0 100644 > --- a/platform/linux-generic/include/odp_packet_io_internal.h > +++ b/platform/linux-generic/include/odp_packet_io_internal.h > @@ -22,6 +22,7 @@ extern "C" { > #include <odp_packet_socket.h> > #include <odp_classification_datamodel.h> > #include <odp_align_internal.h> > +#include <odp_debug_internal.h> > > #include <odp/config.h> > #include <odp/hints.h> > @@ -67,10 +68,13 @@ extern void *pktio_entry_ptr[]; > > static inline pktio_entry_t *get_pktio_entry(odp_pktio_t id) > { > - if (odp_unlikely(id == ODP_PKTIO_INVALID || > - id > ODP_CONFIG_PKTIO_ENTRIES)) > + if (odp_unlikely(id == ODP_PKTIO_INVALID)) > return NULL; > > + if (odp_unlikely(id > ODP_CONFIG_PKTIO_ENTRIES)) > + ODP_ABORT("pktio limit %d/%d exceed\n", > + id, ODP_CONFIG_PKTIO_ENTRIES); > + > > > What is the advantage of introducing an ODP_ABORT() here? The > previous check seemed equally safe and is just rejecting invalid > parameters. My idea was if application requires more pktio entries then odp library have to be reconfigured and recompiled. And the right action is to terminate app completely. Maxim. > return pktio_entry_ptr[id - 1]; > } > #ifdef __cplusplus > diff --git a/platform/linux-generic/odp_packet_io.c > b/platform/linux-generic/odp_packet_io.c > index bdb690e..cfb8d00 100644 > --- a/platform/linux-generic/odp_packet_io.c > +++ b/platform/linux-generic/odp_packet_io.c > @@ -320,7 +320,7 @@ odp_pktio_t odp_pktio_lookup(const > char *dev) > > for (i = 1; i <= ODP_CONFIG_PKTIO_ENTRIES; ++i) { > entry = get_pktio_entry(i); > - if (is_free(entry)) > + if (!entry || is_free(entry)) > > > if (entry == NULL || is_free(entry)) might be clearer. > > continue; > > lock_entry(entry); > -- > 1.8.5.1.163.gd7aced9 > > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > >
Stuart, can you please also review pktio patch? Maxim. ----- Ping one more time. Maxim. On 02/16/2015 06:12 PM, Maxim Uvarov wrote: > That patch was not reviewed. > > Maxim. > > On 2 February 2015 at 15:43, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > CID: 85427 > https://bugs.linaro.org/show_bug.cgi?id=1175 > get_pktio_entry() can return NULL entry then it can be derefenced > inside is_free(entry). > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > platform/linux-generic/include/odp_packet_io_internal.h | 8 ++++++-- > platform/linux-generic/odp_packet_io.c | 2 +- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git > a/platform/linux-generic/include/odp_packet_io_internal.h > b/platform/linux-generic/include/odp_packet_io_internal.h > index 488818c..2c766f0 100644 > --- a/platform/linux-generic/include/odp_packet_io_internal.h > +++ b/platform/linux-generic/include/odp_packet_io_internal.h > @@ -22,6 +22,7 @@ extern "C" { > #include <odp_packet_socket.h> > #include <odp_classification_datamodel.h> > #include <odp_align_internal.h> > +#include <odp_debug_internal.h> > > #include <odp/config.h> > #include <odp/hints.h> > @@ -67,10 +68,13 @@ extern void *pktio_entry_ptr[]; > > static inline pktio_entry_t *get_pktio_entry(odp_pktio_t id) > { > - if (odp_unlikely(id == ODP_PKTIO_INVALID || > - id > ODP_CONFIG_PKTIO_ENTRIES)) > + if (odp_unlikely(id == ODP_PKTIO_INVALID)) > return NULL; > > + if (odp_unlikely(id > ODP_CONFIG_PKTIO_ENTRIES)) > + ODP_ABORT("pktio limit %d/%d exceed\n", > + id, ODP_CONFIG_PKTIO_ENTRIES); > + > return pktio_entry_ptr[id - 1]; > } > #ifdef __cplusplus > diff --git a/platform/linux-generic/odp_packet_io.c > b/platform/linux-generic/odp_packet_io.c > index bdb690e..cfb8d00 100644 > --- a/platform/linux-generic/odp_packet_io.c > +++ b/platform/linux-generic/odp_packet_io.c > @@ -320,7 +320,7 @@ odp_pktio_t odp_pktio_lookup(const char *dev) > > for (i = 1; i <= ODP_CONFIG_PKTIO_ENTRIES; ++i) { > entry = get_pktio_entry(i); > - if (is_free(entry)) > + if (!entry || is_free(entry)) > continue; > > lock_entry(entry); > -- > 1.8.5.1.163.gd7aced9 > >
diff --git a/platform/linux-generic/include/odp_packet_io_internal.h b/platform/linux-generic/include/odp_packet_io_internal.h index 488818c..2c766f0 100644 --- a/platform/linux-generic/include/odp_packet_io_internal.h +++ b/platform/linux-generic/include/odp_packet_io_internal.h @@ -22,6 +22,7 @@ extern "C" { #include <odp_packet_socket.h> #include <odp_classification_datamodel.h> #include <odp_align_internal.h> +#include <odp_debug_internal.h> #include <odp/config.h> #include <odp/hints.h> @@ -67,10 +68,13 @@ extern void *pktio_entry_ptr[]; static inline pktio_entry_t *get_pktio_entry(odp_pktio_t id) { - if (odp_unlikely(id == ODP_PKTIO_INVALID || - id > ODP_CONFIG_PKTIO_ENTRIES)) + if (odp_unlikely(id == ODP_PKTIO_INVALID)) return NULL; + if (odp_unlikely(id > ODP_CONFIG_PKTIO_ENTRIES)) + ODP_ABORT("pktio limit %d/%d exceed\n", + id, ODP_CONFIG_PKTIO_ENTRIES); + return pktio_entry_ptr[id - 1]; } #ifdef __cplusplus diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c index bdb690e..cfb8d00 100644 --- a/platform/linux-generic/odp_packet_io.c +++ b/platform/linux-generic/odp_packet_io.c @@ -320,7 +320,7 @@ odp_pktio_t odp_pktio_lookup(const char *dev) for (i = 1; i <= ODP_CONFIG_PKTIO_ENTRIES; ++i) { entry = get_pktio_entry(i); - if (is_free(entry)) + if (!entry || is_free(entry)) continue; lock_entry(entry);
CID: 85427 https://bugs.linaro.org/show_bug.cgi?id=1175 get_pktio_entry() can return NULL entry then it can be derefenced inside is_free(entry). Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- platform/linux-generic/include/odp_packet_io_internal.h | 8 ++++++-- platform/linux-generic/odp_packet_io.c | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-)